Skip to content

cmd/readmem: bail on u32 address overflow in --file path#707

Open
NeonPhantom123 wants to merge 2 commits into
oxidecomputer:masterfrom
NeonPhantom123:fix/readmem-file-u32-overflow
Open

cmd/readmem: bail on u32 address overflow in --file path#707
NeonPhantom123 wants to merge 2 commits into
oxidecomputer:masterfrom
NeonPhantom123:fix/readmem-file-u32-overflow

Conversation

@NeonPhantom123

@NeonPhantom123 NeonPhantom123 commented Jun 27, 2026

Copy link
Copy Markdown

On the --file path, the address range is computed as:

addr..addr + (length as u32)

Both addr and the result are u32. If addr + length exceeds 0xFFFF_FFFF, the addition wraps silently in release builds, producing an end value smaller than the start. This makes Range<u32> empty, the loop never executes, and the output file is created with zero bytes — while the CLI logs "Wrote N bytes to ..." as if everything succeeded.

This is not a theoretical edge case. Cortex-M peripherals commonly live at high addresses (0xFFFF_0000, 0xE000_0000, etc.). Reading more bytes than the gap between the start address and 0xFFFFFFFF is enough to trigger it. For example:

humility readmem --file out.bin 0xFFFF0000 131072

0xFFFF_0000 + 131072 wraps to 0x0001_0000. The loop runs zero iterations. out.bin is empty. The user sees no error.

The non-file path is already protected against oversized reads by the length > max guard at line 226. The file path has no equivalent guard, and that is what this PR fixes.

The fix computes the end address with u32::try_from + checked_add and bails with a clear error if either overflows, before the file is created or any loop runs.

@andrewjstone

Copy link
Copy Markdown
Contributor

What microcontrollers have you worked with that have more than 4GiB Of RAM?

@NeonPhantom123

Copy link
Copy Markdown
Author

Hey @andrewjstone — fair point, the 4 GiB framing was a distraction and I should have led with the realistic scenario instead.

The actual trigger needs no exotic hardware:

addr   = 0xFFFF_0000   // normal ARM peripheral address
length = 131072        // 128 KiB over two 64 KiB chunks

0xFFFF_0000 + 131072 = 0x1_0001_0000, which wraps to 0x0001_0000 in u32. That produces an empty Range, the loop never executes, the output file is created empty, and the CLI logs "Wrote 131072 bytes" — a silent lie. High addresses in that region are common for vendor peripherals on Cortex-M parts, which is exactly the hardware humility targets.

The non-file path is already protected by the length > max guard at line 226. This fix just brings the --file path to parity.

I'll reopen and update the PR description to drop the 4 GiB angle entirely and lead with this scenario instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants