Skip to content

Security fixes for xrt:api leet audit tickets - #28

Closed
stsoe wants to merge 8 commits into
masterfrom
leet
Closed

stsoe wants to merge 8 commits into
masterfrom
leet

Conversation

@stsoe

@stsoe stsoe commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Problem solved by the commit

Security vulnerabilities identified in the AMD XRT AI Engine and common API
layer by an internal security audit (leet campaign, reporter: obittner,
tickets AIESW-42772–42902). This PR fixes the subset of tickets classified
as xrt:api that were not blocked on other in-progress work.

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

Eight issues fixed, all discovered by internal AI-assisted security audit:

  • AIESW-42872 core/edge/user/shim.cpp: deviceName.copy() used source length instead of destination capacity (char[256]) — classic stack buffer overflow from /etc/xocl.txt.
  • AIESW-42889 core/common/api/hw_queue.cpp: launch() catch handler called pop_back() blindly on submitted_cmds, racing with the monitor thread draining it to running_cmds or a concurrent launch() pushing another entry — UAF on the failed command.
  • AIESW-42890 core/common/api/handle.h: get_or_error() returned const shared_ptr<T>& (a reference into the map) after dropping the mutex — concurrent remove_or_error() could destroy the node leaving a dangling reference. Also fixes downstream callers in xrt_bo.cpp and hip/core/graph.h that re-exposed the reference.
  • AIESW-42839 core/common/api/aie/xrt_graph.cpp: graph_cache and profiling_cache were bare std::map globals with no locking, unlike every other C-API handle table in the directory — concurrent open/close or start/stop caused data races on the red-black tree.
  • AIESW-42834 core/common/api/xrt_xclbin.cpp: init_mems(), init_ips(), and ip_impl::ip_impl() iterated m_count entries from xclbin sections without bounding against actual section size — OOB heap read with attacker-controlled length.
  • AIESW-42776 core/common/api/xrt_xclbin.cpp: ip_impl::ip_impl() passed cxn.arg_index (a raw int32_t from the xclbin) to add_mem_at_idx() without sign or range checks — negative index caused OOB write behind the vector, INT32_MAX caused signed overflow.
  • AIESW-42774 core/common/api/xrt_error.cpp: xrtErrorGetString() computed len-1 without guarding against len==0 — unsigned wraparound to SIZE_MAX bypassed the copy-length limit entirely.
  • AIESW-42773 core/common/api/xrt_kernel.cpp: aie_error_message_v1() iterated ctx_health->aie4.num_uc times without bounding against the packet payload size — device-supplied num_uc could drive reads far past the 4 KB exec buffer.

How problem was solved, alternative solutions (if any) and why they were rejected

Each fix is minimal and local to the affected function:

  • 42872: clamp copy() length to sizeof(info->mName) - 1.
  • 42889: promote running_cmds to a class member (guarded by work_mutex); replace blind pop_back() with targeted find-and-erase from whichever queue still holds the command.
  • 42890: change get_or_error() return type from const shared_ptr<T>& to shared_ptr<T> so the owning copy is made while the mutex is held. Matches the existing get() method.
  • 42839: replace raw std::map instances with xrt_core::handle_map (the existing mutex-protected helper used by all other C-API handle tables).
  • 42834: use get_axlf_section() (which returns {ptr, size}) instead of get_section<T>() (which discards size); clamp loop bounds to (size - sizeof(int32_t)) / sizeof(element).
  • 42776: reject connectivity entries with negative or out-of-range indices by throwing std::runtime_error; cast argidx to size_t before +1 to prevent signed overflow.
  • 42774: return -EINVAL when len == 0 and out is non-null, before the len-1 subtraction.
  • 42773: derive max_uc from epkt->count (fixed overhead 5 words, 11 words per entry); clamp num_uc with std::min before the loop.

Risks (if any) associated the changes in the commit

  • 42890 / 42839: threading behaviour changes. Tested reasoning: get_or_error() by-value is strictly safer (additional refcount); handle_map wraps the same std::map + std::mutex pattern already used everywhere else.
  • 42834 / 42776: xclbin parsing now throws on malformed section data that was previously silently accepted. Legitimate xclbins are unaffected.
  • 42889: running_cmds promoted from monitor-local to class member — no behaviour change under normal operation; only the error path is different.
  • Remaining changes are single-line guards with no effect on the happy path.

What has been tested and how, request additional testing if necessary

  • Code review only at this stage.
  • Please run the full XRT test suite, in particular: xrt::run managed-command tests, xclbin load/parse tests, and any AIE graph multi-thread tests.
  • Edge-case regression: xrtErrorGetString with len==0, xclbin with exactly MAX_KERNELS IP entries.

Documentation impact (if any)

None.

stsoe and others added 8 commits August 23, 2026 19:57
Clamp copy length to sizeof(info->mName) - 1 to prevent stack buffer
overflow when /etc/xocl.txt contains a token longer than 256 bytes.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Replace blind pop_back() in the catch handler with a targeted find-and-
erase of the specific command pointer from either submitted_cmds or
running_cmds, depending on whether the monitor thread has already drained
it. Prevents UAF when submit() throws concurrently with the monitor loop.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Change get_or_error() return type from const shared_ptr<ImplType>& to
shared_ptr<ImplType> so the owning copy is made while the mutex is held.
Returning a reference into the map after dropping the lock allowed a
concurrent xrtDeviceClose() to erase the node, leaving a dangling
reference at the call site.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Replace bare std::map instances for graph_cache and profiling_cache with
the mutex-protected xrt_core::handle_map, matching the pattern used by
xrt_bo.cpp, xrt_kernel.cpp and xrt_xclbin.cpp. Eliminates data races on
concurrent open/close and start/stop of AIE graph and profiling handles.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
In init_mems(), init_ips(), and ip_impl::ip_impl(), use get_axlf_section()
to obtain the section size and clamp iteration to
(size - sizeof(int32_t)) / sizeof(element) entries. sizeof(int32_t) is
both the minimum readable size for m_count and the byte offset to the
flexible array, as m_count is the only non-array member in each struct.
Rejects negative m_count and sections too small to hold the count field.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Reject connectivity entries with negative arg_index or mem_data_index,
or mem_data_index >= mems.size(), by throwing on invalid xclbin data.
Also cast argidx to size_t before +1 in add_mem_at_idx() to prevent
signed integer overflow when argidx == INT32_MAX.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Return -EINVAL when len == 0 and out is non-null. Without this guard,
len-1 wraps to SIZE_MAX, causing memcpy to write the full error string
into the caller's buffer regardless of its capacity.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
Clamp num_uc to the number of ert_uc_health_info entries that fit
within the packet payload before iterating in aie_error_message_v1().
Fixed overhead is 5 uint32_t words (version, npu_gen, ctx_state,
num_uc, ctx_error_type); remaining words divided by entry size gives
the maximum valid entry count.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com>
@stsoe

stsoe commented Aug 24, 2026

Copy link
Copy Markdown
Owner Author

Closing in favour of PR against Xilinx/XRT upstream.

@stsoe stsoe closed this Aug 24, 2026
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