Fix RAM overflow from a crafted TZRULES.BIN, plus seven smaller bounds / ISR-safety bugs

[peterlewis]

Draft / proposal. Eight self-contained bug fixes found while reading through the Mk IV firmware. All are pre-existing issues in master. Two of them — the charger-power hard-fault (#7) and the date-board CMD_LOAD_TEXT OOB (#8) — previously sat in my $PMTXTS PR (#5); I've consolidated them here so every fix lives in one place. Independent of #6 (astro). The changes are small and localised (5 files; about half the added lines are in-place comments explaining each trigger). They've been flashed in a combined build (with #5/#6) and run without regression — see Verification. Left as a draft because they touch core paths (timezone load, USB eject, config parse, CDC); happy to split the Critical (#1) into its own PR if you'd rather take that one first.

The fixes

#	Severity	Where
1	Critical	loadRules()	RAM overflow from a crafted TZRULES.BIN
2	Major	firmwareCheckOnEject()	FATFS re-entered from the USB-eject ISR
3	Minor	sendDate() MODE_TEXT	one-byte overrun of uart2_tx_buffer[32] (time board)
4	Major	latchDisplay() (date board)	out-of-range dp_pos writes outside buffer_a/b[]
5	Minor	rxConfigString()	config-over-USB runs nextMode()/sendDate() in the ISR
6	Minor	readConfigFile()	a zero-timestamp config.txt is never loaded → first-boot hang
7	Major	CDC_Copy_Transmit() / CDC_Transmit_FS()	NULL deref → hard-fault on charger-only power
8	Minor	CMD_LOAD_TEXT (date board)	one-byte overrun of text[32]

1 — loadRules() RAM overflow

TZRULES.BIN lives on the FAT volume the clock exposes over USB MSC, so its bytes are host-writable. loadRules() reads rowLength and numEntries (each a uint8_t) straight from the file, then runs for (i=0; i<numEntries; i++) f_read(&rules[i], rowLength, …) into the fixed rules[MAX_RULES] (162 × 8 B) with no check. With both bytes maxed at 255 a malformed file writes ~990 bytes past the end of the array into adjacent globals — and it does so at cold boot and on every zone re-lookup, so it faults reliably.
Fix: reject the file (RULES_HEADER_ERR) when rowLength > sizeof rules[0] or numEntries > MAX_RULES, before the read loop. Valid files are unaffected.
Repro: on the mounted MSC volume, set the rowLength byte (file offset 4, normally 8) to 0xFF — or any zone's numEntries to > 162 — then trigger a zone load (power-cycle, or move position so the timezone is re-looked-up).

2 — firmwareCheckOnEject() FATFS re-entrancy

This runs from the USB MSC eject command, i.e. in the USB OTG ISR, and calls FATFS (f_open/f_read). It already guards with if (QSPI_Locked()) { delayedCheckOnEject=1; return; }, which defers the whole check to the main loop when it sees QSPI contention — so the design isn't naive. The gap is that QSPI_Locked() is only true during a QSPI transfer; in the lulls between transfers within a single main-loop FATFS call it reads false, the deferral doesn't fire, and the ISR re-enters non-reentrant FATFS anyway (corrupting FATFS state / risking a spurious reset).
Fix: a volatile fatfs_busy flag set around the main-loop FATFS regions; the guard becomes if (fatfs_busy || QSPI_Locked()). It's set/cleared only at the main-loop level, so it stays balanced and can't get stuck and block a legitimate firmware-update-on-eject.

3 — MODE_TEXT one-byte overrun (time board)

i = snprintf((char*)&uart2_tx_buffer[1], 30, "%s", textDisplay) — snprintf returns the length it would have written, not the truncated count. A 31-char TEXT= (the parser caps the value at 31) leaves i == 31, so the following uart2_tx_buffer[++i] = … writes [32], one past the 32-byte buffer, and the DMA length reads one byte OOB. Self-inflicted via config and only one byte, hence Minor — but easy to close.
Fix: if (i > 29) i = 29; — clamp to the bytes the snprintf actually wrote.

4 — date-board dp_pos out-of-bounds

dp_pos comes from text_idx, which the inter-board UART stream can advance past the 10-digit display. latchDisplay() then indexes buffer_a[5]/buffer_b[5] with 9-dp_pos (goes negative when the display is inverted) and dp_pos-6 (overruns when it isn't) → OOB writes into adjacent RAM.
Fix: bail out at the top of the decimal-point block when dp_pos is out of range for the current orientation (> 9 inverted, > 10 otherwise), so neither expression can leave [0,4].

5 — config-over-USB runs nextMode()/sendDate() in the ISR

rxConfigString() runs in the USB OTG ISR (CDC). On each completed line it called postConfigCleanup(), which calls nextMode() and sendDate(). The priority-0 SysTick repaint preempts the USB ISR and calls sendDate(0) directly — so sendDate() is genuinely re-entered — and nextMode() mutates display state (displayMode, the segment buffers) that the repaint reads, so it races too. Result: a torn date row or inconsistent mode state.
Fix: defer postConfigCleanup() to the main loop via a new delayedPostConfigCleanup flag. The file-config path already runs postConfigCleanup() in thread context (because readConfigFile itself is the thing deferred to the main loop); this gives the USB-CDC path the same thread-context guarantee.

6 — zero-timestamp config.txt → first-boot hang

config is {0}-initialised, so the fdate/ftime mtime-cache key starts at 0. A config.txt written while the volume's FAT clock was unset carries a zero timestamp, which matches that initial key on the very first boot → readConfigFile() returns early → config is never applied → no display mode is enabled → the first MODE-button press spins nextMode() forever (there's no watchdog).
Fix: never treat a zero stamp as a cache hit — if ((fno.fdate || fno.ftime) && fno.fdate==config.fdate && fno.ftime==config.ftime) return;.

7 — NMEA-over-CDC NULL deref → hard-fault on charger-only power

CDC_Copy_Transmit() and stock CDC_Transmit_FS() dereference hUsbDeviceFS.pClassDataCDC unconditionally, but it is NULL until a USB host enumerates. With the default nmea_cdc_level = NMEA_ALL, every GPS sentence is forwarded to CDC, so on charger-only power — no host — the first forwarded sentence dereferences NULL in ISR context and hard-faults. This is the most common deployment (a dumb USB charger), so it faults reliably there.
Fix: a NULL guard at the top of both functions (return cleanly when not enumerated), plus a length clamp before the memcpy into the static tx buffer. mk4-time/USB_DEVICE/App/usbd_cdc_if.c.

8 — date-board CMD_LOAD_TEXT one-byte OOB

Distinct from #3 (that's uart2_tx_buffer on the time board; this is the text[] buffer on the date board). CMD_LOAD_TEXT bounds with if (text_idx > MAX_TEXT_LEN) return; then writes text[text_idx++] = x;. At text_idx == MAX_TEXT_LEN (32) the guard passes and it writes text[32], one past the 32-byte array, into the adjacent global. Reachable with a 32-plus-char text-mode string.
Fix: > → >=. mk4-date/Core/Src/main.c.

Verification

• Flashed and running, no regression — built into a combined firmware (with Add $PMTXTS PPS timestamping (opt-in host timing telemetry) #5/Astro pack: sun / moon / Maidenhead / lat-lon display modes #6) and flashed to a real STM32L476 unit; it boots and holds GPS lock with all changes in place. That verifies they don't break normal operation — but I haven't deliberately reproduced each fault's trigger on hardware (e.g. dropping a crafted TZRULES.BIN), so the per-bug case above rests on the code, not on a hardware repro of each fault.
• Each fix was re-checked against the surrounding code for off-by-ones and for not regressing the valid case (a normal TZRULES.BIN/config.txt still loads; fatfs_busy is balanced so it can't latch on).
• No new files and no project/linker changes — the build is structurally unchanged.

Files

• mk4-time/Core/Src/main.c — fixes 1, 3, 5, 6, plus the fatfs_busy wraps for 2
• mk4-time/Core/Src/chainloader.c and Core/Inc/main.h — fix 2
• mk4-time/USB_DEVICE/App/usbd_cdc_if.c — fix 7
• mk4-date/Core/Src/main.c — fixes 4, 8