Security fixes: trace buffer, exec buffer, debug IP layout (leet audit) - #10018
Open
stsoe wants to merge 5 commits into
Open
Security fixes: trace buffer, exec buffer, debug IP layout (leet audit)#10018stsoe wants to merge 5 commits into
stsoe wants to merge 5 commits into
Conversation
Reject indices that exceed the 4096-byte exec buffer capacity before writing. Uses bo_cache::bo_size to derive the word limit at compile time. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Reject add() calls that would exceed the 4096-byte exec buffer before pre-incrementing count and writing addr/value. Guards both the buffer bound and the 11-bit ert_pkt::count field wrap. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Clamp traceSamples to MAX_TRACE_NUMBER_SAMPLES_FIFO in xclGetTraceBufferInfo() and clamp numWords to traceBufWordSz in xclReadTraceData() before the read loop. The primary caller (TraceFifoFull::readTrace()) already clamps nSamples to TRACE_NUMBER_SAMPLES (8192) before calling either function, so the overflow is not reachable through that path. The clamps are retained as defence-in-depth at the shim boundary for any future or direct callers that do not apply the same guard upstream. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Clamp iteration count to the number of debug_ip_data entries that actually fit within the bytes read from sysfs. Uses offsetof() to compute the header size and derives max_entries from bytes_read, then takes std::min with the xclbin-supplied m_count before looping. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Fix signed/unsigned type mismatches in shim.cpp and xrtexec.cpp. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Contributor
|
clang-tidy review says "All clean, LGTM! 👍" |
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
Four security vulnerabilities identified by internal AI-assisted security
audit (leet campaign, reporter: obittner).
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
xrt/xrt++/xrtexec.cpp:exec_cu_command::add()wrotepast a fixed 4096-byte exec BO when
idx >= ~1019— no bounds check onthe caller-supplied register index.
xrt/xrt++/xrtexec.cpp:exec_write_command::add()pre-incremented
ert_pkt->countpast the 1024-word exec buffer after512 calls — no check that two more words fit before writing.
core/pcie/linux/shim.cpp:xclReadTraceData()used afixed-size stack VLA but iterated
numSamples * wordsPerSamplewordswith no bound against the buffer capacity;
xclGetTraceBufferInfo()passed the device-supplied sample count unclamped.
core/pcie/linux/debug.cpp:getIPCountAddrNames()iterated
map->m_countentries (xclbin-controlleduint16_t) from a64 KiB stack buffer with no check against actual bytes read from sysfs.
How problem was solved, alternative solutions (if any) and why they were rejected
bo_cache::bo_sizeto derive the word limit at compile time.traceSamplestoMAX_TRACE_NUMBER_SAMPLES_FIFOinxclGetTraceBufferInfo()and clampnumWordstotraceBufWordSzinxclReadTraceData(). Note: the primary caller already clamps upstream;these are defence-in-depth guards at the shim boundary.
max_entriesfrombytes_readusingoffsetof()andsizeof(debug_ip_data); clamp loop tomin(m_count, max_entries).Risks (if any) associated the changes in the commit
Low. All changes add guards on paths that are either off by default
(trace profiling) or require a crafted xclbin to trigger. Happy paths
are unaffected.
What has been tested and how, request additional testing if necessary
Compile-verified. Please run exec command tests and debug IP status
tests (
xclDebugReadIPStatus) on PCIe devices.Documentation impact (if any)
None.