From 91be146ecbe99fe6d508592fada4d8b0ba227758 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski Date: Fri, 10 Jan 2025 12:33:50 -0800 Subject: [PATCH] CodeQL fixes for "Comparison between A of type TypeA and B of wider type TypeB" --- armsrc/spiffs.h | 1 + armsrc/spiffs_check.c | 7 +++++++ armsrc/spiffs_hydrogen.c | 7 +++++++ armsrc/spiffs_nucleus.c | 9 +++++++++ armsrc/thinfilm.c | 2 +- client/src/cmdhf14a.c | 13 +++++++++++-- 6 files changed, 36 insertions(+), 3 deletions(-) diff --git a/armsrc/spiffs.h b/armsrc/spiffs.h index 7bbfb794b..e0b5868da 100644 --- a/armsrc/spiffs.h +++ b/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 diff --git a/armsrc/spiffs_check.c b/armsrc/spiffs_check.c index c59fcabef..1bd7a2889 100644 --- a/armsrc/spiffs_check.c +++ b/armsrc/spiffs_check.c @@ -536,6 +536,13 @@ static s32_t spiffs_page_consistency_check_i(spiffs *fs) { s32_t res = SPIFFS_OK; spiffs_page_ix pix_offset = 0; + // 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); + SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); + } + // for each range of pages fitting into work memory while (pix_offset < SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count) { // set this flag to abort all checks and rescan the page range diff --git a/armsrc/spiffs_hydrogen.c b/armsrc/spiffs_hydrogen.c index 93c7fbe89..83784b36d 100644 --- a/armsrc/spiffs_hydrogen.c +++ b/armsrc/spiffs_hydrogen.c @@ -52,6 +52,13 @@ s32_t SPIFFS_format(spiffs *fs) { SPIFFS_LOCK(fs); + // 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); + SPIFFS_API_CHECK_RES_UNLOCK(fs, SPIFFS_ERR_INTERNAL); + } + spiffs_block_ix bix = 0; while (bix < fs->block_count) { fs->max_erase_count = 0; diff --git a/armsrc/spiffs_nucleus.c b/armsrc/spiffs_nucleus.c index 2ebfabe04..0cd63ae76 100644 --- a/armsrc/spiffs_nucleus.c +++ b/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,6 +372,14 @@ s32_t spiffs_obj_lu_scan( spiffs_block_ix unerased_bix = (spiffs_block_ix) - 1; #endif + // 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); + SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); + } + + // find out erase count // if enabled, check magic bix = 0; diff --git a/armsrc/thinfilm.c b/armsrc/thinfilm.c index c39316be5..8fd1dd8a8 100644 --- a/armsrc/thinfilm.c +++ b/armsrc/thinfilm.c @@ -115,7 +115,7 @@ 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;) { + for (i = 0; i <= fpga_queued_bits / 8u + 1u;) { if (AT91C_BASE_SSC->SSC_SR & (AT91C_SSC_TXRDY)) { AT91C_BASE_SSC->SSC_THR = SEC_F; FpgaSendQueueDelay = (uint8_t)AT91C_BASE_SSC->SSC_RHR; diff --git a/client/src/cmdhf14a.c b/client/src/cmdhf14a.c index f2d573a8c..df3ceda20 100644 --- a/client/src/cmdhf14a.c +++ b/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) { - uint16_t segment_size = max_rapdu_size < ndef_size + offset - i ? max_rapdu_size : ndef_size + offset - i; + for (size_t i = offset; i < ndef_size + offset; i += max_rapdu_size) { + 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; + + // BUGBUG -- segment_size is stuffed into a single-byte field below? + 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);