Merge pull request #2720 from henrygab/codeQL_fixes2

CodeQL -- Actual fixes

.github/workflows/codeql-analysis.yml

@@ -13,6 +13,7 @@ name: "CodeQL"
 
 on:
   workflow_dispatch:
+
   push:
     branches: [ master ]
   pull_request:

armsrc/spiffs.h

@@ -296,6 +296,7 @@ typedef struct spiffs_t {
     // file system configuration
     spiffs_config cfg;
     // number of logical blocks
+    // BUGBUG -- Should this be of type spiffs_block_ix?
     u32_t block_count;
 
     // cursor for free blocks, block index

armsrc/spiffs_check.c

@@ -535,26 +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 (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) +
+                     (pix_offset * 256) / total_blocks +
-                     ((((cur_block * pages_per_scan * 256) / (SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count))) / fs->block_count),
+                     ((((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 < SPIFFS_PAGES_PER_BLOCK(fs) * (cur_block+1)) {
                 //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;

armsrc/spiffs_hydrogen.c

@@ -52,8 +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 (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) {

armsrc/spiffs_nucleus.c

@@ -364,6 +364,7 @@ static s32_t spiffs_obj_lu_scan_v(
 // Checks magic if enabled
 s32_t spiffs_obj_lu_scan(
     spiffs *fs) {
+
     s32_t res;
     spiffs_block_ix bix;
     int entry;
@@ -371,13 +372,22 @@ 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 (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);
+    }
+
+
     // find out erase count
     // if enabled, check magic
     bix = 0;
     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,

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;
+    uint16_t fpga_queued_bits = FpgaSendQueueDelay >> 3;
-    for (i = 0; i <= (fpga_queued_bits >> 3) + 1;) {
+    fpga_queued_bits >>= 3; // divide by 8 (again?)
+    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;

client/src/cmdhf14a.c

@@ -3226,13 +3226,22 @@ int CmdHF14ANdefRead(const char *Cmd) {
         return PM3_EOVFLOW;
     }
 
-    for (uint16_t i = offset; i < ndef_size + offset; i += max_rapdu_size) {
+    for (size_t i = offset; i < ndef_size + offset; i += max_rapdu_size) {
-        uint16_t segment_size = max_rapdu_size < ndef_size + offset - i ? max_rapdu_size : ndef_size + offset - i;
+        size_t segment_size = max_rapdu_size < ndef_size + offset - i ? max_rapdu_size : ndef_size + offset - i;
+
         keep_field_on = i < ndef_size + offset - max_rapdu_size;
         aREAD_NDEF_n = 0;
         param_gethex_to_eol("00b00000", 0, aREAD_NDEF, sizeof(aREAD_NDEF), &aREAD_NDEF_n);
         aREAD_NDEF[2] = i >> 8;
         aREAD_NDEF[3] = i & 0xFF;
+
+        // Segment_size is stuffed into a single-byte field below ... so error out if overflows
+        if (segment_size > 0xFFu) {
+            PrintAndLogEx(ERR, "Segment size too large (0x%zx > 0xFF)", segment_size);
+            DropField();
+            free(ndef_file);
+            return PM3_EOVFLOW;
+        }
         aREAD_NDEF[4] = segment_size;
 
         res = ExchangeAPDU14a(aREAD_NDEF, aREAD_NDEF_n + 1, activate_field, keep_field_on, response, sizeof(response), &resplen);

client/src/cmdhfmf.c

@@ -117,17 +117,28 @@ static char *GenerateFilename(const char *prefix, const char *suffix) {
     return fptr;
 }
 
+// allocates `items` table entries, storing pointer to `*src`
+// Each entry stores two keys (A and B), initialized to six-byte value 0xFFFFFFFFFFFF
+// Each entry also stores whether the key was "found", defaults to false (0)
 static int initSectorTable(sector_t **src, size_t items) {
 
+
+    // typedef struct {
+    //     uint64_t Key[2];
+    //     uint8_t foundKey[2];
+    // } sector_t;
+
+    // This allocates based on the size of a single item
     (*src) = calloc(items, sizeof(sector_t));
-    if (*src == NULL)
+    if (*src == NULL) {
         return PM3_EMALLOC;
+    }
 
     // empty e_sector
     for (size_t i = 0; i < items; i++) {
         for (uint8_t j = 0; j < 2; j++) {
             (*src)[i].Key[j] = 0xffffffffffff;
-            (*src)[i].foundKey[j] = 0;
+            // (*src)[i].foundKey[j] = 0; // calloc zero's these already
         }
     }
     return PM3_SUCCESS;

client/src/cmdlfem410x.c

@@ -245,7 +245,7 @@ static int ask_em410x_binary_decode(bool verbose, uint32_t *hi, uint64_t *lo, ui
         else if (ans == -4)
             PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x preamble not found");
         else if (ans == -5)
-            PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x Size not correct: %zu", size);
+            PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x Size not correct: %zu", *size);
         else if (ans == -6)
             PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x parity failed");
 

client/src/cmdlfem4x50.c

@@ -30,10 +30,12 @@
 
 static int CmdHelp(const char *Cmd);
 
-static void em4x50_prepare_result(const uint8_t *data, int fwr, int lwr, em4x50_word_t *words) {
+// Each record is 4 bytes long ... a single line in the dump output
+// Reads each record from `data`, reverses the four bytes, and writes to `words`
+static void em4x50_prepare_result(const uint8_t *data, int first_record_inclusive, int last_record_inclusive, em4x50_word_t *words) {
 
     // restructure received result in "em4x50_word_t" structure
-    for (int i = fwr; i <= lwr; i++) {
+    for (int i = first_record_inclusive; i <= last_record_inclusive; i++) {
         for (int j = 0; j < 4; j++) {
             words[i].byte[j] = data[i * 4 + (3 - j)];
         }
@@ -779,6 +781,12 @@ static int CmdEM4x50Reader(const char *Cmd) {
         // iceman,  misuse of return status code.
         int now = resp.status;
 
+        // prevent massive stack corruption if unexpected results from device.
+        if (now > EM4X50_NO_WORDS) {
+            PrintAndLogEx(WARNING, "word count was: %d, limiting to %d", now, EM4X50_NO_WORDS);
+            now = EM4X50_NO_WORDS;
+        }
+
         if (now > 0) {
 
             em4x50_word_t words[EM4X50_NO_WORDS];