SWSPLAT-30710 elf_patcher validate patch offset against BO size before write#9894
Open
stsoe wants to merge 1 commit into
Open
SWSPLAT-30710 elf_patcher validate patch offset against BO size before write#9894stsoe wants to merge 1 commit into
stsoe wants to merge 1 commit into
Conversation
…e write
For each patch_config built from .rela.dyn entries in an attacker-supplied
AIE ELF, offset_to_patch_buffer originates from the ELF relocation r_offset
field. In patch_symbol() the expression:
auto bd_data_ptr = reinterpret_cast<uint32_t*>(base + offset);
was computed with no check that offset + required_patch_bytes <= bo.size().
patch57() then reads/writes 9 words (36 bytes) at that address. A crafted
ELF with r_offset = bo_size - 4 writes 32 bytes past the end of the mapped
instruction BO into adjacent heap memory (CWE-787, CVSS 7.1).
The same unchecked pattern existed in patch_symbol_raw() which takes a raw
pointer with no accompanying size, and in the secondary sync offset computed
for symbol_type::pl_ddr_64.
SWSPLAT-30710. Identified by Mythos AI security audit (MYTHOS-RyzenAI-XRT-H3).
Parent finding: SWSPLAT-30707.
1. Added required_patch_bytes(symbol_type) — a pure helper that returns the
minimum byte span past bd_data_ptr required by each patching scheme,
derived from the highest word index each patch*() function accesses:
- shim_dma_base_addr (patch57): words [1],[2],[8] → span [0..8] = 9 words
- pl_ddr_64 (patch_pl_ddr64): words [8],[9] → span [0..9] = 10 words
- all others: 1–4 words
Magic numbers are suppressed with NOLINT(readability-magic-numbers); each
case has a comment stating the word indices accessed and the index span
required.
2. patch_symbol(): capture bo.size() once, then for every config entry check
offset > bo_size || patch_bytes > bo_size - offset
(overflow-safe form) and throw xrt_core::error(-EINVAL) before computing
bd_data_ptr.
3. The sync lambda is simplified to take no size argument: it reuses the
patch_bytes value already computed for the bounds check, eliminating the
per-case hardcoded byte counts that previously duplicated the information
in required_patch_bytes(). pl_ddr_64 retains an explicit sync call because
its sync starts 8 words into the block rather than at offset, but the
bounds check (10 words) already covers that sub-range.
4. patch_symbol_raw(): added buf_size parameter (callers already know the
buffer size) and apply the identical guard. Updated the one call site in
xrt_module.cpp to pass sz, which is already validated against inst->size()
at that point.
5. Updated elf_patcher.h declaration to match the new signature.
Low. All changes are early-exit guards on attacker-controlled offsets.
Valid ELF files with offsets that fit inside the BO are unaffected.
The buf_size parameter addition is an ABI change to patch_symbol_raw(),
but that function is internal (not exported in the public API).
Code review against the SWSPLAT-30710 finding and manual inspection of
all patch*() word-index accesses to verify required_patch_bytes() covers
each case. AFL++/LibFuzzer harness from the ticket is recommended to
confirm the OOB write is closed.
Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
stsoe
requested review from
HimanshuChoudhary-Xilinx,
chvamshi-xilinx and
larry9523
and removed request for
rbramand-xilinx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return 1 * sizeof(uint32_t); // NOLINT(readability-magic-numbers) | ||
| case symbol_type::shim_dma_base_addr_symbol_kind: | ||
| // words [1],[2],[8] — buffer must cover indices [0..8] | ||
| return 9 * sizeof(uint32_t); // NOLINT(readability-magic-numbers) |
Contributor
There was a problem hiding this comment.
warning: 9 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
]
^| case symbol_type::control_packet_48: | ||
| // words [2],[3] — buffer must cover indices [0..3] | ||
| return 4 * sizeof(uint32_t); // NOLINT(readability-magic-numbers) | ||
| case symbol_type::shim_dma_48: |
Contributor
There was a problem hiding this comment.
warning: switch has 2 consecutive identical branches [bugprone-branch-clone]
rs)
^Additional context
src/runtime_src/core/common/api/elf_patcher.cpp:41: last of these clones ends here
[0..2]
^| return 3 * sizeof(uint32_t); // NOLINT(readability-magic-numbers) | ||
| case symbol_type::pl_ddr_64: | ||
| // words [8],[9] — buffer must cover indices [0..9] | ||
| return 10 * sizeof(uint32_t); // NOLINT(readability-magic-numbers) |
Contributor
There was a problem hiding this comment.
warning: 10 is a magic number; consider replacing it with a named constant [cppcoreguidelines-avoid-magic-numbers]
es [0..9]
^
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem solved by the commit
For each patch_config built from .rela.dyn entries in an attacker-supplied AIE ELF, offset_to_patch_buffer originates from the ELF relocation r_offset field. In patch_symbol() the expression:
auto bd_data_ptr = reinterpret_cast<uint32_t*>(base + offset);
was computed with no check that offset + required_patch_bytes <= bo.size(). patch57() then reads/writes 9 words (36 bytes) at that address. A crafted ELF with r_offset = bo_size - 4 writes 32 bytes past the end of the mapped instruction BO into adjacent heap memory (CWE-787, CVSS 7.1).
The same unchecked pattern existed in patch_symbol_raw() which takes a raw pointer with no accompanying size, and in the secondary sync offset computed for symbol_type::pl_ddr_64.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
SWSPLAT-30710. Identified by Mythos AI security audit (MYTHOS-RyzenAI-XRT-H3). Parent finding: SWSPLAT-30707.
How problem was solved, alternative solutions (if any) and why they were rejected
Added required_patch_bytes(symbol_type) — a pure helper that returns the minimum byte span past bd_data_ptr required by each patching scheme, derived from the highest word index each patch*() function accesses: - shim_dma_base_addr (patch57): words [1],[2],[8] → span [0..8] = 9 words
Magic numbers are suppressed with NOLINT(readability-magic-numbers); each
case has a comment stating the word indices accessed and the index span
required.
patch_symbol(): capture bo.size() once, then for every config entry check offset > bo_size || patch_bytes > bo_size - offset (overflow-safe form) and throw xrt_core::error(-EINVAL) before computing bd_data_ptr.
The sync lambda is simplified to take no size argument: it reuses the patch_bytes value already computed for the bounds check, eliminating the per-case hardcoded byte counts that previously duplicated the information in required_patch_bytes(). pl_ddr_64 retains an explicit sync call because its sync starts 8 words into the block rather than at offset, but the bounds check (10 words) already covers that sub-range.
patch_symbol_raw(): added buf_size parameter (callers already know the buffer size) and apply the identical guard. Updated the one call site in xrt_module.cpp to pass sz, which is already validated against inst->size() at that point.
Updated elf_patcher.h declaration to match the new signature.
Risks (if any) associated the changes in the commit
Low. All changes are early-exit guards on attacker-controlled offsets. Valid ELF files with offsets that fit inside the BO are unaffected. The buf_size parameter addition is an ABI change to patch_symbol_raw(), but that function is internal (not exported in the public API).
What has been tested and how, request additional testing if necessary
Code review against the SWSPLAT-30710 finding and manual inspection of all patch*() word-index accesses to verify required_patch_bytes() covers each case. AFL++/LibFuzzer harness from the ticket is recommended to confirm the OOB write is closed.