bugfix: integer underflow in PSRAM virtual-disk file import (OOB write from hostile FAT metadata)

[Amiga500]

Summary

psram_copy_file() and psram_mmap_file() (stm32/COLDCARD_MK4/psramdisk.c, shared by Q1 via symlink) trust FAT metadata of the PSRAM virtual disk, which is exposed over USB Mass Storage and therefore fully writable by a compromised USB host.

The final fragment length is computed as:

len = actual_len - so_far;   // uint32_t arithmetic

where actual_len = fp.obj.objsize (directory entry size field) and so_far is the sum of the bytes covered by previous cluster-chain fragments. FatFs' CREATE_LINKMAP follows the cluster chain to EOC without validating it against objsize, so a crafted FAT (chain covering more bytes than objsize, e.g. on a .dfu/.psbt file dropped by the host) makes so_far > actual_len and len underflows to ~4 GB. The subsequent memcpy(dest, spot, len) then writes attacker-influenced data out of bounds:

• the pre-write check dest + actual_len + 3 >= PSRAM_TOP_BASE is insufficient and can itself wrap around for objsize near 4 GB;
• the dest >= PSRAM_TOP_BASE check happened after the memcpy — too late.

Realistic impact: corruption of everything staged in PSRAM (PSBT/firmware staging areas) and device crash via BusFault. SRAM and flash sit below 0x90000000, so the forward-only copy does not reach them; no credible path to code execution was identified (hence moderate severity), but it is a deterministic host-triggered memory corruption reachable without user confirmation when Virtual Disk is in "Enable & Auto" mode.

Fix

• Reject cluster chains whose coverage exceeds the file size (so_far > actual_len, and per-fragment len > remaining) before the subtraction — no more underflow.
• Verify destination bounds with 64-bit arithmetic before each memcpy, eliminating both the post-hoc check and the wrap-around.
• Same guard applied to psram_mmap_file(), which hands attacker-influenced (address, length) tuples to MicroPython.

Verification

Logic validated with a standalone host harness replicating the fixed loop: legit single/multi-fragment files pass; hostile chains (oversized coverage, objsize near 4 GB, degenerate zero-size) are all rejected. A full firmware build was not possible in the authoring environment (no ARM toolchain / uninitialized submodules) — a run through repro-build.sh before merge would be appreciated.

[scgbckbone]

are you planning to re-open ?

[Amiga500]

I have sent an email 6 hours ago to security@coinkite.com with all info

[Amiga500]

but i can reopen it right away without any problems if you give me the ok

[doc-hex]

Ok, to reopen this PR... it's very not hidden at this point.

This fix can be in the next release as it didn't make today's cutoff.

[scgbckbone]

Two hostile FAT/link-map shapes can still bypass these guards and produce out-of-bounds PSRAM reads or mappings. The fragment count must be validated before it is narrowed by block_to_ptr(), and the declared final remainder must be checked against the final fragment's physical capacity.

[scgbckbone]

ACK cf3fe22

nit: the changelog only mentions oversized chains and OOB writes while the patch also prevents undersized final fragments and OOB reads/mappings - please update that wording and resolve the changelog conflict with master

otherwise LGTM

[doc-hex]

Two findings from reviewing the current patch:

1. Fragment capacity assumes one sector per cluster. CREATE_LINKMAP reports run lengths in clusters, but num_clusters * BLOCK_SIZE and block_to_ptr(..., num_clusters) treat the count as sectors. Because the USB host can rewrite the BPB, FatFs can mount a valid filesystem with csize > 1; such files can then be rejected or, for fragmented/overallocated layouts, copied or mapped with skipped data. Please either compute a checked sector count as num_clusters * vfs.fatfs.csize and use it for both block_to_ptr() and capacity, or explicitly reject csize != 1 after mounting.

2. One trailing fragment is still accepted when preceding fragments exactly cover the declared size. If so_far == objsize, the final branch produces len == 0 and accepts the oversized chain; mmap() consequently returns a spurious zero-length tuple. Please reject this case when another fragment remains (including a zero-size file with an allocated chain).

Non-blocking follow-ups: the PR currently conflicts with master in Next-ChangeLog.md, and the hostile-FAT cases are only covered by an external harness rather than repository regression tests.

[Amiga500]

Thanks for the careful re-review. Both findings addressed in 457acd8:

1. Sectors vs. clusters (csize > 1) — I went with the explicit-reject option: right after f_mount(), both psram_mmap_file() and psram_copy_file() now refuse any mounted filesystem with vfs.fatfs.csize != 1 (ValueError("bad clus size")). I preferred this over computing a checked sector count (num_clusters * csize) because the whole downstream math — clst2sect()'s scaling, block_to_ptr()'s sector range check, and the capacity bookkeeping — is built around 512-byte sectors; pinning csize == 1 keeps one invariant instead of threading a second unit through every calculation. A host-rewritten BPB now just makes the file unreadable instead of mis-mapped.

2. Trailing fragment after exact coverage — the final branch now rejects len < 1 as well as len > capacity:

len = fp.obj.objsize - so_far;
if((len < 1) || (len > capacity)) {
mp_raise_ValueError(MP_ERROR_TEXT("bad file size"));
}

So when preceding fragments already cover objsize exactly (so_far == objsize), a remaining fragment is rejected as oversized-chain metadata instead of producing a spurious zero-length tuple from mmap(). This also covers the zero-size-file-with-allocated-chain case you mentioned (objsize == 0 with any link map).

Follow-ups:

The Next-ChangeLog.md conflict with master is resolved in the current head (both entries kept), and the entry now also mentions spurious trailing fragments and multi-sector clusters.
On the regression tests: agreed that repo-side coverage would be better than my external harness. The catch is these paths run on bare-metal PSRAM/FAT code with no simulator hook today, so a pytest-style regression would need either a host-compiled harness of psramdisk.c or new simulator plumbing. Happy to add the host-harness variant under testing/ if you think that's worth the build plumbing — let me know your preference.

Re-validated with the standalone harness extended for the new shapes: legit single/multi-fragment files (including normal over-allocation of the final cluster) pass; hostile shapes (csize 2/4, num_clusters u16 wrap, oversized chains, exact-coverage trailing fragment, zero-size file with chain, clusters past end of disk) are all rejected. CodeQL is clean on the diff.

[doc-hex]

Move commit 457acd8 onto the branch backing the PR. The fixes currently exist only on copilot/amiga500-fix-psramdisk-fat-underflow, while PR 763 tracks amiga500-fix-psramdisk-fat-underflow.

[Amiga500]

@doc-hex Done — the fixes from 457acd8 are now on the PR branch as be54bf8 (cherry-picked onto the PR head; tree verified identical). Ready for re-review.

[scgbckbone]

remove