CodeQL -- Actual fixes #2720

henrygab · 2025-01-15T04:13:55Z

Many of the CodeQL bugs of type "Comparison between A of TypeA and B of wider type TypeB" were caused by having arithmetic within a for() loop's comparison clause. Not only were the operations redundant, but they could cause unintended consequences (e.g., integer promotion rules). Relatively simple to fix (use larger of two sizes for loop variable, compare unsigned vs. unsigned, etc.).

The other reason for many of these was that spiffs, in the definition of struct spiffs_t, defined field block_count of type uint32_t, but defined field free_cursor_block_ix to be of type spiffs_block_ix. In PM3, spiffs_block_ix is defined as uint16_t. Thus, a variable of type spiffs_block_ix cannot directly loop against block_count (potential infinite loops).

Inline "conversation"s added below, both to explain some changes, and to mark code that should have a second set of eyes review.

…ype TypeB"

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.

The compiler warning is incorrect. Since `calloc()` zero's memory, can remove redundant line setting value to zero, giving quieter builds.

henrygab · 2025-01-15T04:18:01Z

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)


Please have second set of eyes review these checks.

henrygab · 2025-01-15T04:19:59Z

armsrc/thinfilm.c

@@ -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 >> 3) + 1;) {
+    uint16_t fpga_queued_bits = FpgaSendQueueDelay >> 3;


explicitly use unsigned values for the maths, and store back as uint16_t. This allows the for() loop to index on a variable of size uint16_t.

henrygab · 2025-01-15T04:21:28Z

client/src/cmdhf14a.c

        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 previous code ever had a segment_size greater than 0xFF, then the old code would have silently truncated the value. After review, this appeared to be incorrect behavior, and that there was an implicit assumption that segment_size would always be single-byte value.

Proper solution would not to use size_t but u32 and look into the ndef standard to handle larger than 0xFF sized segments?

henrygab · 2025-01-15T04:22:40Z

client/src/cmdhfmf.c


    // 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


No, I cannot explain why CodeQL alerted on this line, and therefore cannot explain why its removal makes CodeQL happier. But, it's redundant since calloc() ensures memory is zero at allocation, so ... removal is OK.

true , still shows that codeQL has false positives

henrygab · 2025-01-15T04:23:07Z

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);


size was a pointer to the size, not the size itself.

This is supported for GCC >= version 13 See GCC bug 85487: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85487

iceman1001 · 2025-01-15T14:21:51Z

armsrc/spiffs_check.c

                     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) {


this one says "cur_block + 1" vs "total_blocks_plus_one_page" ???

iceman1001 · 2025-01-15T14:34:37Z

armsrc/spiffs_hydrogen.c

    spiffs_block_ix bix = 0;
-    while (bix < fs->block_count) {
+    while (bix < block_count) {


is it really that block_count is large, or ?
block_count = 0, nothing happens,
if large number, the spiff_ok check should fail when trying to erase a block outside of existing once..

not sure how this would be a infinite loop?

iceman1001 · 2025-01-15T14:36:33Z

armsrc/thinfilm.c

-    uint8_t fpga_queued_bits = FpgaSendQueueDelay >> 3;
-    for (i = 0; i <= (fpga_queued_bits >> 3) + 1;) {
+    uint16_t fpga_queued_bits = FpgaSendQueueDelay >> 3;
+    fpga_queued_bits /= 8u;


use the >>> 3 for consistency. Once upon a time we didn't want to trigger floats inside the arm code because of size.

iceman1001 · 2025-01-15T14:46:53Z

client/src/cmdhfmf.c

+
+    // This allocates based on the size of a single item
+    _Static_assert(sizeof(sector_t) >= 18, "Unexpectedly small sector_t"); // if packed, would be 18
+    _Static_assert(sizeof(sector_t) == 24, "Sector_t used to be padded to 24 bytes?"); // not packed, so each entry must be 24 bytes


This code looks more like debug code.

// ISSUE: C99 has no _Static_assert() ... was added in C11

iceman1001 · 2025-01-15T14:48:45Z

client/src/cmdlfem4x50.c

+            PrintAndLogEx(WARNING, "word count was: %d, limiting to %d", now, EM4X50_NO_WORDS);
+            now = EM4X50_NO_WORDS;
+        }
+
        if (now > 0) {



this one can be " if (now) {

iceman1001 · 2025-01-15T14:49:31Z

client/src/cmdlfem4x50.c

+            PrintAndLogEx(WARNING, "word count was: %d, limiting to %d", now, EM4X50_NO_WORDS);
+            now = EM4X50_NO_WORDS;
+        }
+


the proper fix would be not misusing the return status code and check it accordingly

henrygab added 8 commits January 14, 2025 17:52

Mismatched format string specifier (need to deref size pointer)

4d67f56

no-op

20c4756

CodeQL fixes for "Comparison between A of type TypeA and B of wider t…

91be146

…ype TypeB"

Prevent potential stack corruption

18f6604

Quiet a spurious warning

1c3f845

The compiler warning is incorrect. Since `calloc()` zero's memory, can remove redundant line setting value to zero, giving quieter builds.

improve comment

0531946

fix build break on some compilers

99e8de3

henrygab commented Jan 15, 2025

View reviewed changes

Workaround old GCC on Windows builds

6613ff7

This is supported for GCC >= version 13 See GCC bug 85487: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85487

iceman1001 reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CodeQL -- Actual fixes #2720

CodeQL -- Actual fixes #2720

henrygab commented Jan 15, 2025 •

edited

Loading

henrygab Jan 15, 2025

henrygab Jan 15, 2025

iceman1001 Jan 15, 2025

henrygab Jan 15, 2025

iceman1001 Jan 15, 2025

henrygab Jan 15, 2025

iceman1001 Jan 15, 2025

henrygab Jan 15, 2025

iceman1001 Jan 15, 2025

iceman1001 Jan 15, 2025

iceman1001 Jan 15, 2025

iceman1001 Jan 15, 2025

iceman1001 Jan 15, 2025

iceman1001 Jan 15, 2025

iceman1001 Jan 15, 2025

CodeQL -- Actual fixes #2720

Are you sure you want to change the base?

CodeQL -- Actual fixes #2720

Conversation

henrygab commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrygab commented Jan 15, 2025 •

edited

Loading