From 1c75690b1a51abe5a1addf5633ffa59557e0416f Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jan 2025 15:02:19 -0800 Subject: [PATCH] Various codeQL fixes Code was previously performing arithmetic in various loop check conditions. Integer promotion rules could cause unintended comparisons. `spiffs` defined `fs->block_count` as `uint32_t`, but defined `spiffs_page_ix` as `uint16_t`. Various overflow checks detected by CodeQL and fixed by checking for those conditions before looping. --- armsrc/spiffs_check.c | 44 ++++++++++++++++++++++++++++++++-------- armsrc/spiffs_hydrogen.c | 7 ++++--- armsrc/spiffs_nucleus.c | 7 ++++--- armsrc/thinfilm.c | 8 +++++--- 4 files changed, 49 insertions(+), 17 deletions(-) diff --git a/armsrc/spiffs_check.c b/armsrc/spiffs_check.c index 1bd7a2889..8a190b8ac 100644 --- a/armsrc/spiffs_check.c +++ b/armsrc/spiffs_check.c @@ -535,33 +535,61 @@ static s32_t spiffs_page_consistency_check_i(spiffs *fs) { s32_t res = SPIFFS_OK; spiffs_page_ix pix_offset = 0; + // Avoid arithmetic in loop conditions (integer promotion rules can cause unintended consequences) + uint32_t block_count = fs->block_count; + uint32_t total_blocks = SPIFFS_PAGES_PER_BLOCK(fs) * block_count; + uint32_t total_blocks_plus_one_page = total_blocks + SPIFFS_PAGES_PER_BLOCK(fs); +#pragma region // check for overflow once, before looping // this _should_ never happen, but prefer to see debug message / error // rather than silently entering infinite loop. - if (fs->block_count > ((spiffs_block_ix)(-1))) { - SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", fs->block_count); + if (block_count > ((spiffs_block_ix)(-1))) { + SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", block_count); SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); } + // this checks for overflow of the multiplication of block_count+1 with SPIFFS_PAGES_PER_BLOCK(fs) + if (((uint32_t)(-1)) / SPIFFS_PAGES_PER_BLOCK(fs) > (block_count+1)) { + // checking with +1 block count to avoid overflow also in inner loop, which adds one page... + // would exceed value storable in uint32_t + SPIFFS_DBG("Overflow: pages per block %04x with block count "_SPIPRIbl" results in overflow\n", SPIFFS_PAGES_PER_BLOCK(fs), block_count); + SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); + } + // because loop indices are using spiffs_page_ix type, + // that type can hold a large enough value + if (total_blocks > ((spiffs_page_ix)-1)) { + SPIFFS_DBG("Avoiding infinite loop, total_blocks "_SPIPRIpg" too large for spiffs_page_ix type\n", total_blocks); + SPIFFS_CHECK_RES(SPIFFS_ERR_INTERNAL); + } + // because loop indices are using spiffs_page_ix type, + // that type can hold a large enough value + if (total_blocks_plus_one_page > ((spiffs_page_ix)-1) || total_blocks_plus_one_page < total_blocks) { + SPIFFS_DBG("Avoiding infinite loop, total_blocks_plus_one_page "_SPIPRIpg" too large for spiffs_page_ix type\n", total_blocks_plus_one_page); + SPIFFS_CHECK_RES(SPIFFS_ERR_INTERNAL); + } + // RESULT: spiffs_page_ix can safely be used for loop index vs. each of + // block_count, total_blocks, and total_blocks_plus_one_page +#pragma endregion // check for overflow once, before looping + // for each range of pages fitting into work memory - while (pix_offset < SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count) { + while (pix_offset < total_blocks) { // set this flag to abort all checks and rescan the page range u8_t restart = 0; memset(fs->work, 0, SPIFFS_CFG_LOG_PAGE_SZ(fs)); spiffs_block_ix cur_block = 0; // build consistency bitmap for id range traversing all blocks - while (!restart && cur_block < fs->block_count) { + while (!restart && cur_block < block_count) { CHECK_CB(fs, SPIFFS_CHECK_PAGE, SPIFFS_CHECK_PROGRESS, - (pix_offset * 256) / (SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count) + - ((((cur_block * pages_per_scan * 256) / (SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count))) / fs->block_count), + (pix_offset * 256) / total_blocks + + ((((cur_block * pages_per_scan * 256) / total_blocks)) / block_count), 0); // traverse each page except for lookup pages spiffs_page_ix cur_pix = SPIFFS_OBJ_LOOKUP_PAGES(fs) + SPIFFS_PAGES_PER_BLOCK(fs) * cur_block; - while (!restart && cur_pix < SPIFFS_PAGES_PER_BLOCK(fs) * (cur_block + 1)) { + while (!restart && cur_pix < total_blocks_plus_one_page) { //if ((cur_pix & 0xff) == 0) // SPIFFS_CHECK_DBG("PA: processing pix "_SPIPRIpg", block "_SPIPRIbl" of pix "_SPIPRIpg", block "_SPIPRIbl"\n", - // cur_pix, cur_block, SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count, fs->block_count); + // cur_pix, cur_block, total_blocks, block_count); // read header spiffs_page_header p_hdr; diff --git a/armsrc/spiffs_hydrogen.c b/armsrc/spiffs_hydrogen.c index 83784b36d..9f44cff13 100644 --- a/armsrc/spiffs_hydrogen.c +++ b/armsrc/spiffs_hydrogen.c @@ -52,15 +52,16 @@ s32_t SPIFFS_format(spiffs *fs) { SPIFFS_LOCK(fs); + uint32_t block_count = fs->block_count; // this _should_ never happen, but prefer to see debug message / error // rather than silently entering infinite loop. - if (fs->block_count > ((spiffs_block_ix)(-1))) { - SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", fs->block_count); + if (block_count > ((spiffs_block_ix)(-1))) { + SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", block_count); SPIFFS_API_CHECK_RES_UNLOCK(fs, SPIFFS_ERR_INTERNAL); } spiffs_block_ix bix = 0; - while (bix < fs->block_count) { + while (bix < block_count) { fs->max_erase_count = 0; s32_t res = spiffs_erase_block(fs, bix); if (res != SPIFFS_OK) { diff --git a/armsrc/spiffs_nucleus.c b/armsrc/spiffs_nucleus.c index 0cd63ae76..cd6c708c4 100644 --- a/armsrc/spiffs_nucleus.c +++ b/armsrc/spiffs_nucleus.c @@ -372,10 +372,11 @@ s32_t spiffs_obj_lu_scan( spiffs_block_ix unerased_bix = (spiffs_block_ix) - 1; #endif + uint32_t block_count = fs->block_count; // this _should_ never happen, but prefer to see debug message / error // rather than silently entering infinite loop. - if (fs->block_count > ((spiffs_block_ix)(-1))) { - SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", fs->block_count); + if (block_count > ((spiffs_block_ix)(-1))) { + SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", block_count); SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); } @@ -386,7 +387,7 @@ s32_t spiffs_obj_lu_scan( spiffs_obj_id erase_count_final; spiffs_obj_id erase_count_min = SPIFFS_OBJ_ID_FREE; spiffs_obj_id erase_count_max = 0; - while (bix < fs->block_count) { + while (bix < block_count) { #if SPIFFS_USE_MAGIC spiffs_obj_id magic; res = _spiffs_rd(fs, diff --git a/armsrc/thinfilm.c b/armsrc/thinfilm.c index 8fd1dd8a8..efea4c899 100644 --- a/armsrc/thinfilm.c +++ b/armsrc/thinfilm.c @@ -105,7 +105,7 @@ static int EmSendCmdThinfilmRaw(const uint8_t *resp, uint16_t respLen) { uint16_t FpgaSendQueueDelay = 0; // send cycle - uint16_t i = 0; + size_t i = 0; for (; i < respLen;) { if (AT91C_BASE_SSC->SSC_SR & (AT91C_SSC_TXRDY)) { AT91C_BASE_SSC->SSC_THR = resp[i++]; @@ -114,8 +114,10 @@ static int EmSendCmdThinfilmRaw(const uint8_t *resp, uint16_t respLen) { } // Ensure that the FPGA Delay Queue is empty - uint8_t fpga_queued_bits = FpgaSendQueueDelay >> 3; - for (i = 0; i <= fpga_queued_bits / 8u + 1u;) { + uint16_t fpga_queued_bits = FpgaSendQueueDelay >> 3; + fpga_queued_bits /= 8u; + fpga_queued_bits += 1u; + for (i = 0; i <= fpga_queued_bits;) { if (AT91C_BASE_SSC->SSC_SR & (AT91C_SSC_TXRDY)) { AT91C_BASE_SSC->SSC_THR = SEC_F; FpgaSendQueueDelay = (uint8_t)AT91C_BASE_SSC->SSC_RHR;