Skip to content

iFlashRead() overflows caller buffer by up to 7 bytes on final unaligned iteration#27

Open
merkelmarrow wants to merge 1 commit intoXilinx:mainfrom
merkelmarrow:fix/iFlashRead-unaligned-overflow
Open

iFlashRead() overflows caller buffer by up to 7 bytes on final unaligned iteration#27
merkelmarrow wants to merge 1 commit intoXilinx:mainfrom
merkelmarrow:fix/iFlashRead-unaligned-overflow

Conversation

@merkelmarrow
Copy link
Copy Markdown

@merkelmarrow merkelmarrow commented Apr 22, 2026

Context

iFlashRead() (fw/AMC/src/device_drivers/ospi/aved/ospi.c) reads flash into a caller buffer. In the function there is one variable, ulBytesToRead, that is reused for three different purposes during a single call:

  1. On entry: it holds the requested length (e.g. 12 bytes, how much the caller wants).
  2. In the unaligned branch: the function overwrites it with OSPI_READ_BUFFER_SIZE (= 8), because each individual flash transfer is always 8 bytes at a time.
  3. On the last loop iteration: it should be rewritten again to the leftover tail (e.g. 4), so the final memcpy doesn't copy a full 8 bytes into a buffer that only had 4 bytes of room.

The bug is that step 3 happens after the memcpy it was supposed to constrain. By that point the overflow has already occurred.

Impact

The copy overruns pucReadBfrPtr by up to 7 bytes for any read with a length not a multiple of 8. On every V80 cold boot, iLoadFptPartition() asks for exactly 12 bytes (sizeof(APC_PROXY_DRIVER_FPT_PARTITION) == 12), and the overflow writes 4 bytes, overwriting iLoadFptPartition's iStatus with zeros (verified via disassembly + DWARF, the overwrite value is always 0x00 because of the way gen_fpt.py zero-initialises the FPT image).

This does nothing on a successful read since iStatus returns zero anyway. On a failed read the memcpy never gets triggered.

But when the firmware is compiled with stack protections, this bug surfaces as NO_AMC in ami_tool overview due to the stack canary trip during boot.

Stack frame layout is unspecified by the C standard so this bug could be more consequential in other setups. Any future changes to the code could also result in other stack locals being silently overwritten.

Fix

The fix is to snapshot the post-clamp length (ulEffectiveBytes) before the unaligned branch overwrites ulBytesToRead, and on the final iteration compute ulBytesToRead = ulEffectiveBytes − (i * OSPI_READ_BUFFER_SIZE) before the memcpy. Using the post-clamp length rather than ulByteCount % 8 keeps the trim correct in the stacked-flash-tail case (>256 Mbit flashes).

On the final iteration of an unaligned iFlashRead(), ulBytesToRead was
trimmed to (ulByteCount % OSPI_READ_BUFFER_SIZE) *after* the pvOSAL_MemCpy
into the caller's buffer, so the copy overran pucReadBfrPtr by up to 7 bytes
for any read with a length not a multiple of 8.

If stack hardening isn't enabled at compile-time, this is easy to go
unnoticed. On every V80 cold boot, iLoadFptPartition() asks for exactly
12 bytes, and the overflow writes 4 bytes which overwrites iStatus with
zeros. This does nothing on a successful read since iStatus returns
zero anyway.

The fix is to compute the trim before memcpy from a snapshot of the
post-clamp length taken before the unaligned branch overwrites it.

Signed-off-by: Marco Blackwell <mblackwe@amd.com>
@merkelmarrow merkelmarrow force-pushed the fix/iFlashRead-unaligned-overflow branch from 38fc880 to 0e6a3cd Compare April 22, 2026 13:37
@merkelmarrow merkelmarrow marked this pull request as ready for review April 22, 2026 13:37
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.

1 participant