From 9b98cc6794950f7eba17543f0cea4ca2e9e5aef0 Mon Sep 17 00:00:00 2001 From: Soren Soe <2106410+stsoe@users.noreply.github.com> Date: Sat, 4 Jul 2026 14:53:25 -0700 Subject: [PATCH] SWSPLAT-30708 xrt_kernel validate arg offset/size against execbuf payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hs_arg_setter::set_arg_value() and set_offset_value() write caller-supplied bytes into data + arg.offset() where data points into the 4096-byte exec BO payload. arg.offset() and arg.size() are parsed from the xclbin EMBEDDED_METADATA XML with no upper bound. A malicious xclbin with offset=0x10000 causes every set_arg() call to write past the mapped BO into adjacent heap memory (CWE-787, CVSS 7.1). fa_arg_setter has the same defect via fa_desc_offset(). SWSPLAT-30708. Identified by Mythos AI security audit (MYTHOS-RyzenAI-XRT-H2). Parent finding: SWSPLAT-30705. Added payload_size() to run_impl that computes the true number of bytes available for kernel arguments in the command buffer: reserved = bytes consumed by ert header + cu masks (computed from data ptr) payload = kernel_command::allocation_size - reserved (physical ceiling) if regmap_size > 0: cap to regmap_size bytes (validates it fits in physical) if regmap_size == 0: unlimited args — use physical ceiling This handles kernels where regmap_size is not set (unlimited arguments) while still bounding writes to the actual buffer. The resulting payload_size is passed to each arg_setter at construction time as m_payload_size. Overflow-safe bounds checks are added before every copy_n in hs_arg_setter (set_arg_value, set_offset_value, get_arg_value) and before the fa_desc_offset pointer arithmetic in fa_arg_setter. bo_cache.h is amended to expose bo_size as a public constexpr so kernel_command::allocation_size can reference it without a magic number. All four make_arg_setter() sites (pl/ps/dpu kernels and mailbox_impl) updated. Low. The bounds check fires only when arg.offset() + count exceeds the payload — impossible for a well-formed xclbin. Malformed inputs that previously caused silent heap corruption now throw xrt_core::error(-EINVAL). Code review against the SWSPLAT-30708 finding. Existing kernel argument tests exercise the nominal path. The AFL++ harness from the ticket (feed xclbin bytes to xrt::kernel + run.set_arg) is recommended to confirm the OOB write is closed. Co-Authored-By: Claude Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com> --- .../core/common/api/xrt_kernel.cpp | 85 ++++++++++++++----- src/runtime_src/core/common/bo_cache.h | 13 ++- 2 files changed, 72 insertions(+), 26 deletions(-) diff --git a/src/runtime_src/core/common/api/xrt_kernel.cpp b/src/runtime_src/core/common/api/xrt_kernel.cpp index 1f55a5824dd..9368446e7f1 100644 --- a/src/runtime_src/core/common/api/xrt_kernel.cpp +++ b/src/runtime_src/core/common/api/xrt_kernel.cpp @@ -869,7 +869,7 @@ class kernel_command : public xrt_core::command using execbuf_type = xrt_core::bo_cache::cmd_bo; using callback_function_type = std::function; using callback_list = std::vector; - + static constexpr size_t allocation_size = xrt_core::bo_cache::bo_size; private: // Return state of underlying exec buffer packet This is an // asynchronous call, the command object may not be in the same @@ -2281,10 +2281,11 @@ class run_impl : public std::enable_shared_from_this struct arg_setter : argument::setter { uint8_t* data; + size_t m_payload_size; // bytes available in payload starting at data - explicit - arg_setter(uint32_t* d) + arg_setter(uint32_t* d, size_t payload_size) : data(reinterpret_cast(d)) + , m_payload_size(payload_size) {} virtual @@ -2315,9 +2316,8 @@ class run_impl : public std::enable_shared_from_this // AP_CTRL_HS, AP_CTRL_CHAIN struct hs_arg_setter : arg_setter { - explicit - hs_arg_setter(uint32_t* data) - : arg_setter(data) + hs_arg_setter(uint32_t* data, size_t payload_size) + : arg_setter(data, payload_size) {} void @@ -2325,6 +2325,9 @@ class run_impl : public std::enable_shared_from_this { // max 4 bytes supported for direct register write auto count = std::min(4, value.size()); + if (offset > m_payload_size || count > m_payload_size - offset) + throw xrt_core::error(-EINVAL, "kernel arg offset/size exceeds execbuf payload"); + std::copy_n(value.begin(), count, data + offset); } @@ -2332,12 +2335,18 @@ class run_impl : public std::enable_shared_from_this set_arg_value(const argument& arg, const arg_range& value) override { auto count = std::min(arg.size(), value.size()); + if (arg.offset() > m_payload_size || count > m_payload_size - arg.offset()) + throw xrt_core::error(-EINVAL, "kernel arg offset/size exceeds execbuf payload"); + std::copy_n(value.begin(), count, data + arg.offset()); } arg_range get_arg_value(const argument& arg) override { + if (arg.offset() > m_payload_size || arg.size() > m_payload_size - arg.offset()) + throw xrt_core::error(-EINVAL, "kernel arg offset/size exceeds execbuf payload"); + return { data + arg.offset(), arg.size() }; } }; @@ -2345,9 +2354,8 @@ class run_impl : public std::enable_shared_from_this // FAST_ADAPTER struct fa_arg_setter : arg_setter { - explicit - fa_arg_setter(uint32_t* data) - : arg_setter(data) + fa_arg_setter(uint32_t* data, size_t payload_size) + : arg_setter(data, payload_size) {} void @@ -2359,7 +2367,12 @@ class run_impl : public std::enable_shared_from_this void set_arg_value(const argument& arg, const arg_range& value) override { + auto fa_offset = arg.fa_desc_offset() / sizeof(uint32_t) * sizeof(uint32_t); auto desc = reinterpret_cast(data); + if (fa_offset > m_payload_size - offsetof(ert_fa_descriptor, data) + || sizeof(ert_fa_desc_entry) > m_payload_size - offsetof(ert_fa_descriptor, data) - fa_offset) + throw xrt_core::error(-EINVAL, "kernel fa_desc_offset exceeds execbuf payload"); + auto desc_entry = reinterpret_cast(desc->data + arg.fa_desc_offset() / sizeof(uint32_t)); desc_entry->arg_offset = arg.offset(); desc_entry->arg_size = arg.size(); @@ -2370,7 +2383,12 @@ class run_impl : public std::enable_shared_from_this arg_range get_arg_value(const argument& arg) override { + auto fa_offset = arg.fa_desc_offset() / sizeof(uint32_t) * sizeof(uint32_t); auto desc = reinterpret_cast(data); + if (fa_offset > m_payload_size - offsetof(ert_fa_descriptor, data) + || sizeof(ert_fa_desc_entry) > m_payload_size - offsetof(ert_fa_descriptor, data) - fa_offset) + throw xrt_core::error(-EINVAL, "kernel fa_desc_offset exceeds execbuf payload"); + auto desc_entry = reinterpret_cast(desc->data + arg.fa_desc_offset() / sizeof(uint32_t)); return { reinterpret_cast(desc_entry->arg_value), arg.size() }; } @@ -2379,9 +2397,8 @@ class run_impl : public std::enable_shared_from_this // PS_KERNEL struct ps_arg_setter : hs_arg_setter { - explicit - ps_arg_setter(uint32_t* data) - : hs_arg_setter(data) + ps_arg_setter(uint32_t* data, size_t payload_size) + : hs_arg_setter(data, payload_size) {} void @@ -2418,18 +2435,48 @@ class run_impl : public std::enable_shared_from_this hwctx, ctrl_code_id, ctrlpkt_bo); } + // payload_size() - Number of bytes in command payload that can be + // used for kernel arguments. + size_t + payload_size() const + { + // run_impl::data is offset from cmd->get_ert_cmd() computed + // during command initialization. The command buffer size is + // kernel_command::allocation_size (4K). + // Total number of bytes available for payload is size of command + // buffer minus number of bytes assigned during command + // initialization. + auto reserved = (data - cmd->get_ert_cmd()) * sizeof(uint32_t); + auto payload_size = kernel_command::allocation_size - reserved; + + // If regmap_size is initialized, it holds the number of words + // used by command arguments, which must not be greater than + // the remaining payload size + if (auto regmap_size = kernel->get_regmap_size()) { + auto regmap_bytes = regmap_size * sizeof(uint32_t); + if (regmap_bytes > payload_size) + throw xrt_core::error(-EINVAL, "kernel register map exceeed command payload size"); + + return regmap_bytes; + } + + // Without regmap_size, unlimitted number of arguments is + // supported, but must not exceeed remaining payload size + return payload_size; + } + virtual std::unique_ptr make_arg_setter() { switch (kernel->get_kernel_type()) { case kernel_type::pl : if (kernel->get_ip_control_protocol() == control_type::fa) - return std::make_unique(data); - return std::make_unique(data); + return std::make_unique(data, payload_size()); + return std::make_unique(data, payload_size()); case kernel_type::ps : - return std::make_unique(data); + return std::make_unique(data, payload_size()); case kernel_type::dpu : - return std::make_unique(data); + return std::make_unique(data, payload_size()); case kernel_type::none : throw std::runtime_error("Internal error: unknown kernel type"); } @@ -3279,8 +3326,8 @@ class mailbox_impl : public run_impl mailbox_impl* mbox; static constexpr size_t wsize = sizeof(uint32_t); // register word size - hs_arg_setter(uint32_t* data, mailbox_impl* mimpl) - : run_impl::hs_arg_setter(data), data32(data), mbox(mimpl) + hs_arg_setter(uint32_t* data, size_t payload_size, mailbox_impl* mimpl) + : run_impl::hs_arg_setter(data, payload_size), data32(data), mbox(mimpl) {} void @@ -3479,7 +3526,7 @@ class mailbox_impl : public run_impl if (kernel->get_ip_control_protocol() == control_type::fa) throw xrt_core::error("Mailbox not supported with FAST_ADAPTER"); - return std::make_unique(data, this); // data is run_impl::data + return std::make_unique(data, payload_size(), this); // data is run_impl::data } throw xrt_core::error("Mailbox not supported for non pl kernel types"); diff --git a/src/runtime_src/core/common/bo_cache.h b/src/runtime_src/core/common/bo_cache.h index 079b467cf44..582702b70b6 100644 --- a/src/runtime_src/core/common/bo_cache.h +++ b/src/runtime_src/core/common/bo_cache.h @@ -24,18 +24,17 @@ namespace xrt_core { // Create a cache of CMD BO objects -- for now only used for M2M -- to reduce // the overhead of BO life cycle management. template -class bo_cache_t { +class bo_cache_t +{ public: // Helper typedef for std::pair. Note the elements are const so that the // pair is immutable. The clients should not change the contents of cmd_bo. template using cmd_bo = std::pair, CommandType *const>; -private: + // Expose bo size in bytes to clients who use default bo_cache type + static constexpr size_t bo_size = BoSize * 1024u; // NOLINT - // We are really allocating a page size as that is what xocl/zocl do. Note on - // POWER9 pagesize maybe more than 4K, xocl would upsize the allocation to the - // correct pagesize. unmap always unmaps the full page. - static constexpr size_t m_bo_size = BoSize; +private: std::shared_ptr m_device; // Maximum number of BOs that can be cached in the pool. Value of 0 indicates // caching should be disabled. @@ -92,7 +91,7 @@ class bo_cache_t { } } - auto execHandle = m_device->alloc_bo(m_bo_size, XCL_BO_FLAGS_EXECBUF); + auto execHandle = m_device->alloc_bo(bo_size, XCL_BO_FLAGS_EXECBUF); auto map = execHandle->map(buffer_handle::map_type::write); return std::make_pair(std::move(execHandle), map); }