From d5d00bce0af08cd5337a911a50f2fbb6f81c5ad8 Mon Sep 17 00:00:00 2001 From: Antiklesys Date: Sun, 10 May 2026 14:35:16 +0800 Subject: [PATCH] smartcard-i2c-fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## armsrc: smartcard / I2C correctness + SAM compatibility fixes A focused pass over `armsrc/i2c.{c,h}` and `armsrc/i2c_direct.c` addressing correctness bugs (NULL deref on alloc failure, unbounded recursion, write-failure silently falling through to read), and removing two long-standing SAM-compatibility blockers (`SmartCardSetBaud` was a stub; float in firmware hot path). ### Fixed - **`SmartCardRaw`: BigBuf_calloc result was unchecked.** NULL deref on memory pressure crashed the firmware. Now bails with `PM3_EMALLOC` before touching `resp[]`. - **`SmartCardRaw`: `I2C_BufferWrite` failure was only acted on at `dbglevel > 3`.** Lower debug levels silently fell through to `sc_rx_bytes()` reading from a card we never wrote to. The `reply_ng` / `goto OUT` path now runs on any write failure; the `Dbprintf` is the only thing gated on dbglevel. - **`SmartCardRaw`: replaced float division** `(p->wait_delay * 1000) / 3.07` with integer math via `uint64_t`. Avoids pulling `__aeabi_fdiv` into the ARM image and removes overflow at large `wait_delay` values: `wait = (ms * 100000 + 153) / 307`. - **`SmartCardUpgrade`: BigBuf_calloc result was unchecked.** Now bails with `PM3_EMALLOC`. Renamed local `verfiydata` typo to `verifydata`. Failure messages now include the offset where the upgrade aborted (`Writing failed at offset 0x%04X` instead of `Writing failed`). - **`I2C_BufferRead`: rejected `len == 0` but accepted `len == 1`**, which cannot represent the SIM module's 2-byte BE length header (returned -1 cast through int16_t). Tightened to `len < 2`. - **`WaitSCL_L_timeout`: `return (delay == 0)` was dead code** — post-decrement underflows `delay` to `UINT32_MAX` after the loop, so the expression was always false. Replaced with explicit `return false` and corrected the comment that claimed an 1800 ms cap (actual cap is 1200 ms). - **`i2c.h`: SIM_WAIT_DELAY comment was wrong** — `150000 * 3.07us = 460 ms`, not 270 ms. Updated comment to match the actual value; constant unchanged. - **`SmartCardDirectSend`: BigBuf_calloc result for `resp[]` was unchecked.** The next `resp[0] = prepend` would NULL-deref on alloc failure. Now bails. - **`SmartCardDirectSend`: GET RESPONSE (61xx) chain recursed without bound**, allocating a fresh `smart_card_raw_t` from BigBuf each round. A misbehaving card returning 61xx every time would wedge the device. Added `depth` parameter capped at `SC_DIRECT_MAX_DEPTH` (8); both call sites updated. The GET RESPONSE inner allocation is also now NULL-checked. - **`SmartCardDirectSend`: same write-failure-silent fix** as `SmartCardRaw`. - **`SmartCardDirectSend`: same float → integer-math conversion** as `SmartCardRaw`. ### Added - **`SmartCardSetBaud` is now functional.** Was an empty stub (wired through `pm3_cmd.h` / `appmain.c` dispatch but did nothing). Now sends `I2C_DEVICE_CMD_SETBAUD` (0x04) to the SIM module's main address. ### Notes for follow-up - `SmartCardSetBaud` is now functional on the firmware side, but no client command exposes it — `client/src/cmdsmartcard.c` has no `setbaud` subcommand. A separate change can wire `CMD_SMART_SETBAUD` into the client if needed; leaving it out of this patch keeps it firmware-only and safe to backport. - The proxmark3 SIM module's stock asm firmware (`sim013`) silently drops `CMD_SETBAUD`. gentilkiwi's `sim_c` v4.50+ also currently dispatches it to a no-op. Full PPS follow-through requires a SIM-firmware-side patch in addition to this commit. - Mid-byte SCL stretch timeouts in `I2C_SendByte` / `I2C_ReadByte` are still swallowed (return void / -1, both ignored at most callers). Fixing this requires changing those signatures and verifying every caller — out of scope here, merits its own PR. --- armsrc/i2c.c | 59 +++++++++++++++++++++++++++++++++------------ armsrc/i2c.h | 8 +++--- armsrc/i2c_direct.c | 41 +++++++++++++++++++++++++------ 3 files changed, 82 insertions(+), 26 deletions(-) diff --git a/armsrc/i2c.c b/armsrc/i2c.c index 89106bb99..d5994969d 100644 --- a/armsrc/i2c.c +++ b/armsrc/i2c.c @@ -187,9 +187,8 @@ static bool WaitSCL_L(void) { return WaitSCL_L_delay(5000); } -// Wait max 1800ms or until SCL goes LOW. -// It timeout reading response from card -// Which ever comes first +// Wait up to 1200ms or until SCL goes LOW, whichever comes first. +// Used to bound the timeout while reading a response from the card. static bool WaitSCL_L_timeout(void) { volatile uint32_t delay = 1200; while (delay--) { @@ -199,7 +198,7 @@ static bool WaitSCL_L_timeout(void) { WaitMS(1); } - return (delay == 0); + return false; } static bool I2C_Start(void) { @@ -489,8 +488,9 @@ bool I2C_BufferWrite(const uint8_t *data, uint16_t len, uint8_t device_cmd, uint // len = uint16 because we need to read up to 256bytes int16_t I2C_BufferRead(uint8_t *data, uint16_t len, uint8_t device_cmd, uint8_t device_address) { - // sanity check - if (data == NULL || len == 0) { + // sanity check - need at least 2 bytes for the SIM-module length header + // (the response format prepends a 2-byte BE length); fewer cannot be parsed. + if (data == NULL || len < 2) { return 0; } @@ -858,7 +858,11 @@ void SmartCardRaw(const smart_card_raw_t *p) { uint16_t len = 0; uint8_t *resp = BigBuf_calloc(ISO7816_MAX_FRAME); - // check if alloacted... + if (resp == NULL) { + reply_ng(CMD_SMART_RAW, PM3_EMALLOC, NULL, 0); + LEDsoff(); + return; + } smartcard_command_t flags = p->flags; if ((flags & SC_CLEARLOG) == SC_CLEARLOG) @@ -888,7 +892,10 @@ void SmartCardRaw(const smart_card_raw_t *p) { uint32_t wait = SIM_WAIT_DELAY; if ((flags & SC_WAIT) == SC_WAIT) { - wait = (uint32_t)((p->wait_delay * 1000) / 3.07); + // wait_delay is in ms; one WaitSCL_H_delay iteration is ~3.07us. + // Integer-only conversion via uint64_t to avoid soft-float and avoid + // overflow at large wait_delay values: (ms * 100000 + 153) / 307. + wait = (uint32_t)(((uint64_t)p->wait_delay * 100000U + 153U) / 307U); } LogTrace(p->data, p->len, 0, 0, NULL, true); @@ -900,8 +907,10 @@ void SmartCardRaw(const smart_card_raw_t *p) { I2C_DEVICE_ADDRESS_MAIN ); - if (res == false && g_dbglevel > 3) { - DbpString(I2C_ERROR); + if (res == false) { + if (g_dbglevel > 3) { + DbpString(I2C_ERROR); + } reply_ng(CMD_SMART_RAW, PM3_ESOFT, NULL, 0); goto OUT; } @@ -937,7 +946,12 @@ void SmartCardUpgrade(uint64_t arg0) { bool isOK = true; uint16_t length = arg0, pos = 0; const uint8_t *fwdata = BigBuf_get_addr(); - uint8_t *verfiydata = BigBuf_calloc(I2C_BLOCK_SIZE); + uint8_t *verifydata = BigBuf_calloc(I2C_BLOCK_SIZE); + if (verifydata == NULL) { + reply_ng(CMD_SMART_UPGRADE, PM3_EMALLOC, NULL, 0); + LED_C_OFF(); + return; + } while (length) { @@ -951,7 +965,7 @@ void SmartCardUpgrade(uint64_t arg0) { // write int16_t res = I2C_WriteFW(fwdata + pos, size, msb, lsb, I2C_DEVICE_ADDRESS_BOOT); if (!res) { - DbpString("Writing failed"); + Dbprintf("Writing failed at offset 0x%04X", pos); isOK = false; break; } @@ -960,16 +974,16 @@ void SmartCardUpgrade(uint64_t arg0) { WaitMS(50); // read - res = I2C_ReadFW(verfiydata, size, msb, lsb, I2C_DEVICE_ADDRESS_BOOT); + res = I2C_ReadFW(verifydata, size, msb, lsb, I2C_DEVICE_ADDRESS_BOOT); if (res <= 0) { - DbpString("Reading back failed"); + Dbprintf("Reading back failed at offset 0x%04X", pos); isOK = false; break; } // cmp - if (0 != memcmp(fwdata + pos, verfiydata, size)) { - DbpString("not equal data"); + if (0 != memcmp(fwdata + pos, verifydata, size)) { + Dbprintf("Verify mismatch at offset 0x%04X", pos); isOK = false; break; } @@ -983,7 +997,20 @@ void SmartCardUpgrade(uint64_t arg0) { BigBuf_free(); } +// Send a single byte to the SIM module's CMD_SETBAUD opcode (0x04). +// The 8051 firmware uses this to reload Timer1 (UART0 baud generator). +// Until 2026 the implementation was an empty stub; the SIM module silently +// ignored any host-driven baud renegotiation. Some smart cards (notably the +// HID Artemis SLE88 SAM family) advertise non-default Fi/Di in TA1 and need +// PPS to switch the bridge baud post-ATR. void SmartCardSetBaud(uint64_t arg0) { + LED_D_ON(); + I2C_Reset_EnterMainProgram(); + bool ok = I2C_WriteByte((uint8_t)(arg0 & 0xFF), + I2C_DEVICE_CMD_SETBAUD, + I2C_DEVICE_ADDRESS_MAIN); + reply_ng(CMD_SMART_SETBAUD, ok ? PM3_SUCCESS : PM3_ESOFT, NULL, 0); + LEDsoff(); } void SmartCardSetClock(uint64_t arg0) { diff --git a/armsrc/i2c.h b/armsrc/i2c.h index b45832acd..be2ced3f4 100644 --- a/armsrc/i2c.h +++ b/armsrc/i2c.h @@ -34,9 +34,11 @@ #define ISO7816_MAX_FRAME 270 // 8051 speaks with smart card. -// 1000*50*3.07 = 153.5ms -// 1 byte transfer == 1ms with max frame being 256 bytes -#define SIM_WAIT_DELAY 150000 // about 270ms delay // 109773 -- about 337.7ms delay +// 1 byte transfer == 1ms with max frame being 256 bytes. +// SIM_WAIT_DELAY is the iteration count passed to WaitSCL_H_delay(); each iter +// is ~3.07us, so 150000 * 3.07us = ~460ms - the upper bound we wait for the +// SIM module to assert SCL after a SIM-side operation. +#define SIM_WAIT_DELAY 150000 // ~460ms total wait via WaitSCL_H_delay void I2C_recovery(void); diff --git a/armsrc/i2c_direct.c b/armsrc/i2c_direct.c index 49aaa4c2c..6685cac59 100644 --- a/armsrc/i2c_direct.c +++ b/armsrc/i2c_direct.c @@ -36,13 +36,25 @@ #include "i2c.h" #include "i2c_direct.h" -static void SmartCardDirectSend(uint8_t prepend, const smart_card_raw_t *p, uint8_t *output, uint16_t *olen) { +// Maximum chained ISO 7816-4 GET RESPONSE (61xx) follow-ups before bailing. +// A misbehaving card that always returns 61xx would otherwise recurse without +// bound, allocating fresh smart_card_raw_t payloads from BigBuf each time +// until the device wedges. Eight rounds is more than any legitimate APDU +// chain would need. +#define SC_DIRECT_MAX_DEPTH 8 + +static void SmartCardDirectSend(uint8_t prepend, const smart_card_raw_t *p, uint8_t *output, uint16_t *olen, uint8_t depth) { LED_D_ON(); uint16_t len = 0; uint8_t *resp = BigBuf_calloc(ISO7816_MAX_FRAME); + if (resp == NULL) { + Dbprintf("SmartCardDirectSend: BigBuf_calloc failed"); + if (olen) *olen = 0; + LEDsoff(); + return; + } resp[0] = prepend; - // check if alloacted... smartcard_command_t flags = p->flags; if ((flags & SC_LOG) == SC_LOG) @@ -70,7 +82,10 @@ static void SmartCardDirectSend(uint8_t prepend, const smart_card_raw_t *p, uint if (((flags & SC_RAW) == SC_RAW) || ((flags & SC_RAW_T0) == SC_RAW_T0)) { if ((flags & SC_WAIT) == SC_WAIT) { - wait = (uint32_t)((p->wait_delay * 1000) / 3.07); + // wait_delay is in ms; one WaitSCL_H_delay iteration is ~3.07us. + // Integer-only conversion via uint64_t to avoid soft-float and + // avoid overflow at large wait_delay values. + wait = (uint32_t)(((uint64_t)p->wait_delay * 100000U + 153U) / 307U); } LogTrace(p->data, p->len, 0, 0, NULL, true); @@ -82,8 +97,10 @@ static void SmartCardDirectSend(uint8_t prepend, const smart_card_raw_t *p, uint I2C_DEVICE_ADDRESS_MAIN ); - if (res == false && g_dbglevel > 3) { - Dbprintf("SmartCardDirectSend: I2C_BufferWrite failed\n"); + if (res == false) { + if (g_dbglevel > 3) { + Dbprintf("SmartCardDirectSend: I2C_BufferWrite failed"); + } goto OUT; } @@ -98,15 +115,25 @@ static void SmartCardDirectSend(uint8_t prepend, const smart_card_raw_t *p, uint } if (len == 2 && resp[1] == 0x61) { + + if (depth >= SC_DIRECT_MAX_DEPTH) { + Dbprintf("SmartCardDirectSend: GET RESPONSE chain depth (%u) exceeded; aborting", depth); + goto OUT; + } + uint8_t cmd_getresp[] = {0x00, ISO7816_GET_RESPONSE, 0x00, 0x00, resp[2]}; smart_card_raw_t *payload = (smart_card_raw_t *)BigBuf_calloc(sizeof(smart_card_raw_t) + sizeof(cmd_getresp)); + if (payload == NULL) { + Dbprintf("SmartCardDirectSend: GET RESPONSE alloc failed"); + goto OUT; + } payload->flags = SC_RAW | SC_LOG; payload->len = sizeof(cmd_getresp); payload->wait_delay = 0; memcpy(payload->data, cmd_getresp, sizeof(cmd_getresp)); - SmartCardDirectSend(prepend, payload, output, olen); + SmartCardDirectSend(prepend, payload, output, olen, depth + 1); } else if (len == 2) { Dbprintf("***** BAD response from card (response unsupported)..."); Dbhexdump(3, &resp[0], false); @@ -199,7 +226,7 @@ int CmdSmartRaw(const uint8_t prepend, const uint8_t *data, int dlen, uint8_t *o payload->flags |= SC_RAW; } - SmartCardDirectSend(prepend, payload, output, olen); + SmartCardDirectSend(prepend, payload, output, olen, 0); return PM3_SUCCESS; }