Skip to content

Adding OOB checks in the xclbin_parser #9589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ManojTakasi wants to merge 1 commit into
base: master
Choose a base branch
from
+117 −34
Open

Adding OOB checks in the xclbin_parser #9589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 73 additions & 9 deletions src/runtime_src/core/common/xclbin_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
* Copyright (C) 2026 Advanced Micro Devices, Inc. All rights reserved.
* Copyright (C) 2019-2022 Xilinx, Inc
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may
Expand Down Expand Up @@ -835,13 +836,28 @@ get_softkernels(const axlf* top)
pSection != nullptr;
pSection = ::xclbin::get_axlf_section_next(top, pSection, SOFT_KERNEL)) {
auto begin = reinterpret_cast<const char*>(top) + pSection->m_sectionOffset;
auto section_size = pSection->m_sectionSize;

if (section_size < sizeof(soft_kernel))
continue;

auto soft = reinterpret_cast<const soft_kernel*>(begin);

if (soft->mpo_symbol_name >= section_size ||
soft->mpo_name >= section_size ||
soft->mpo_version >= section_size ||
soft->m_image_offset >= section_size ||
(soft->m_image_offset + soft->m_image_size) > section_size)
Comment on lines +849 to +850
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check (soft->m_image_offset + soft->m_image_size) > section_size can be bypassed via unsigned integer overflow if the fields are file-controlled. Use an overflow-safe pattern instead (e.g., validate m_image_size <= section_size and m_image_offset <= section_size - m_image_size).

Suggested change
soft->m_image_offset >= section_size ||
(soft->m_image_offset + soft->m_image_size) > section_size)
soft->m_image_size > section_size ||
soft->m_image_offset > section_size - soft->m_image_size)

Copilot uses AI. Check for mistakes.
continue;

softkernel_object sko;
sko.ninst = soft->m_num_instances;
sko.symbol_name = std::string(begin + soft->mpo_symbol_name);
sko.mpo_name = std::string(begin + soft->mpo_name);
sko.mpo_version = std::string(begin + soft->mpo_version);
sko.symbol_name = std::string(begin + soft->mpo_symbol_name,
strnlen(begin + soft->mpo_symbol_name, section_size - soft->mpo_symbol_name));
sko.mpo_name = std::string(begin + soft->mpo_name,
strnlen(begin + soft->mpo_name, section_size - soft->mpo_name));
sko.mpo_version = std::string(begin + soft->mpo_version,
strnlen(begin + soft->mpo_version, section_size - soft->mpo_version));
Comment on lines +855 to +860
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These bounded reads prevent OOB, but can still copy very large substrings if the binary omits NUL terminators and the remaining section size is large. Consider adding a small maximum string length cap (like in tooling metadata) to avoid excessive allocations/copies from malformed inputs.

Copilot uses AI. Check for mistakes.
sko.size = soft->m_image_size;
sko.sk_buf = const_cast<char*>(begin + soft->m_image_offset); // NOLINT
sks.emplace_back(std::move(sko));
Expand All @@ -858,29 +874,77 @@ get_aie_partition(const axlf* top)
return {};

auto topbase = reinterpret_cast<const char*>(top) + pSection->m_sectionOffset;
auto section_size = pSection->m_sectionSize;

if (section_size < sizeof(aie_partition))
return {};

auto aiep = reinterpret_cast<const aie_partition*>(topbase);
auto scp = reinterpret_cast<const uint16_t*>(topbase + aiep->info.start_columns.offset);

aie_partition_obj obj{aiep->info.column_width, {scp, scp + aiep->info.start_columns.size}, topbase + aiep->mpo_name, aiep->operations_per_cycle};
if (aiep->mpo_name >= section_size)
return {};
if (aiep->info.start_columns.offset >= section_size)
return {};
size_t start_cols_bytes = static_cast<size_t>(aiep->info.start_columns.size) * sizeof(uint16_t);
if (start_cols_bytes > section_size ||
aiep->info.start_columns.offset > section_size - start_cols_bytes)
return {};
if (aiep->aie_pdi.offset >= section_size)
return {};
size_t aie_pdi_bytes = static_cast<size_t>(aiep->aie_pdi.size) * sizeof(aie_pdi);
if (aie_pdi_bytes > section_size || aiep->aie_pdi.offset > section_size - aie_pdi_bytes)
Comment on lines +888 to +895
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several size computations/offset+size checks are still overflow-prone with attacker-controlled values: multiplications like size * sizeof(T) can wrap, and additions like offset + size can wrap. Replace with overflow-safe comparisons (e.g., size > section_size / sizeof(T) before multiplying, and offset > section_size - size instead of offset + size > section_size) for start_cols_bytes, aie_pdi_bytes, cdo_groups_bytes, ids_bytes, and pdi_image.offset + pdi_image.size.

Suggested change
size_t start_cols_bytes = static_cast<size_t>(aiep->info.start_columns.size) * sizeof(uint16_t);
if (start_cols_bytes > section_size ||
aiep->info.start_columns.offset > section_size - start_cols_bytes)
return {};
if (aiep->aie_pdi.offset >= section_size)
return {};
size_t aie_pdi_bytes = static_cast<size_t>(aiep->aie_pdi.size) * sizeof(aie_pdi);
if (aie_pdi_bytes > section_size || aiep->aie_pdi.offset > section_size - aie_pdi_bytes)
if (aiep->info.start_columns.size > section_size / sizeof(uint16_t))
return {};
size_t start_cols_bytes = static_cast<size_t>(aiep->info.start_columns.size) * sizeof(uint16_t);
if (aiep->info.start_columns.offset > section_size - start_cols_bytes)
return {};
if (aiep->aie_pdi.offset >= section_size)
return {};
if (aiep->aie_pdi.size > section_size / sizeof(aie_pdi))
return {};
size_t aie_pdi_bytes = static_cast<size_t>(aiep->aie_pdi.size) * sizeof(aie_pdi);
if (aiep->aie_pdi.offset > section_size - aie_pdi_bytes)

Copilot uses AI. Check for mistakes.
return {};

auto scp = reinterpret_cast<const uint16_t*>(topbase + aiep->info.start_columns.offset);
aie_partition_obj obj{aiep->info.column_width, {scp, scp + aiep->info.start_columns.size},
std::string(topbase + aiep->mpo_name, strnlen(topbase + aiep->mpo_name, section_size - aiep->mpo_name)),
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These bounded reads prevent OOB, but can still copy very large substrings if the binary omits NUL terminators and the remaining section size is large. Consider adding a small maximum string length cap (like in tooling metadata) to avoid excessive allocations/copies from malformed inputs.

Copilot uses AI. Check for mistakes.
aiep->operations_per_cycle};

for (uint32_t i = 0; i < aiep->aie_pdi.size; i++) {
aie_pdi_obj pdiobj;
auto aiepdip = reinterpret_cast<const aie_pdi*>(topbase + aiep->aie_pdi.offset + i * sizeof(aie_pdi));
size_t pdi_offset = aiep->aie_pdi.offset + i * sizeof(aie_pdi);
if (pdi_offset + sizeof(aie_pdi) > section_size)
continue;

auto aiepdip = reinterpret_cast<const aie_pdi*>(topbase + pdi_offset);

if (aiepdip->pdi_image.offset >= section_size ||
(aiepdip->pdi_image.offset + aiepdip->pdi_image.size) > section_size ||
aiepdip->cdo_groups.offset >= section_size)
continue;
size_t cdo_groups_bytes = static_cast<size_t>(aiepdip->cdo_groups.size) * sizeof(cdo_group);
if (cdo_groups_bytes > section_size ||
aiepdip->cdo_groups.offset > section_size - cdo_groups_bytes)
continue;
Comment on lines +910 to +917
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several size computations/offset+size checks are still overflow-prone with attacker-controlled values: multiplications like size * sizeof(T) can wrap, and additions like offset + size can wrap. Replace with overflow-safe comparisons (e.g., size > section_size / sizeof(T) before multiplying, and offset > section_size - size instead of offset + size > section_size) for start_cols_bytes, aie_pdi_bytes, cdo_groups_bytes, ids_bytes, and pdi_image.offset + pdi_image.size.

Copilot uses AI. Check for mistakes.

if (aiepdip->pdi_image.size > PDI_IMAGE_MAX_SIZE)
throw std::runtime_error("PDI image size too big");

aie_pdi_obj pdiobj;
pdiobj.uuid = aiepdip->uuid;
pdiobj.pdi.resize(aiepdip->pdi_image.size);
memcpy(pdiobj.pdi.data(), topbase + aiepdip->pdi_image.offset, pdiobj.pdi.size());

for (uint32_t j = 0; j < aiepdip->cdo_groups.size; j++) {
size_t cdo_offset = aiepdip->cdo_groups.offset + j * sizeof(cdo_group);
if (cdo_offset + sizeof(cdo_group) > section_size)
continue;

auto cdop = reinterpret_cast<const cdo_group*>(topbase + cdo_offset);
if (cdop->mpo_name >= section_size ||
cdop->dpu_kernel_ids.offset >= section_size)
continue;
size_t ids_bytes = static_cast<size_t>(cdop->dpu_kernel_ids.size) * sizeof(uint64_t);
if (ids_bytes > section_size || cdop->dpu_kernel_ids.offset > section_size - ids_bytes)
continue;
Comment on lines +936 to +938
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several size computations/offset+size checks are still overflow-prone with attacker-controlled values: multiplications like size * sizeof(T) can wrap, and additions like offset + size can wrap. Replace with overflow-safe comparisons (e.g., size > section_size / sizeof(T) before multiplying, and offset > section_size - size instead of offset + size > section_size) for start_cols_bytes, aie_pdi_bytes, cdo_groups_bytes, ids_bytes, and pdi_image.offset + pdi_image.size.

Copilot uses AI. Check for mistakes.

std::vector<uint64_t> dpu_kernel_ids;
auto cdop = reinterpret_cast<const cdo_group*>(topbase + aiepdip->cdo_groups.offset + j * sizeof(cdo_group));
auto kernel_idp = reinterpret_cast<const uint64_t*>(topbase + cdop->dpu_kernel_ids.offset);
for (uint32_t k = 0; k < cdop->dpu_kernel_ids.size; ++k)
dpu_kernel_ids.push_back(kernel_idp[k]);

pdiobj.cdo_groups.emplace_back<aie_cdo_group_obj>({topbase + cdop->mpo_name, cdop->cdo_type, cdop->pdi_id, std::move(dpu_kernel_ids)});
std::string cdo_name(topbase + cdop->mpo_name,
strnlen(topbase + cdop->mpo_name, section_size - cdop->mpo_name));
Comment on lines +945 to +946
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These bounded reads prevent OOB, but can still copy very large substrings if the binary omits NUL terminators and the remaining section size is large. Consider adding a small maximum string length cap (like in tooling metadata) to avoid excessive allocations/copies from malformed inputs.

Copilot uses AI. Check for mistakes.
pdiobj.cdo_groups.emplace_back<aie_cdo_group_obj>({cdo_name, cdop->cdo_type, cdop->pdi_id, std::move(dpu_kernel_ids)});
}

obj.pdis.emplace_back(std::move(pdiobj));
Expand Down
16 changes: 12 additions & 4 deletions src/runtime_src/core/tools/common/xball
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ exit $python_exit_code
import json
import os
import re
import subprocess
import shlex
import sys

temp_python_status = sys.argv[1]
Expand Down Expand Up @@ -198,11 +200,17 @@ for device in working_devices:
print("\n")
print("=====================================================================")
print("%d / %d [%s] : %s" % (device_count, len(working_devices), device["bdf"], device["vbnv"]))
cmd = xrt_bin_dir + "/" + xrt_app + " --device " + device["bdf"] + " " + prog_args
cmd_list = [os.path.join(xrt_bin_dir, xrt_app), "--device", device["bdf"]]
if prog_args:
cmd_list.extend(shlex.split(prog_args))
print("~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~")
print("Command: %s\n" % cmd)
exit_code = os.system(cmd)

print("Command: %s\n" % " ".join(shlex.quote(a) for a in cmd_list))
try:
exit_code = subprocess.run(cmd_list).returncode
except OSError as e:
print("Error: %s" % e)
exit_code = 1

if exit_code == 0:
print("\nCommand Return Value: 0 [Operation successful]");
passed_devices += 1
Expand Down
53 changes: 32 additions & 21 deletions src/runtime_src/tools/xclbinutil/SectionAIEResourcesBin.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@

/**
* Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.
* Copyright (C) 2026 Advanced Micro Devices, Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"). You may
* not use this file except in compliance with the License. A copy of the
Expand All @@ -22,6 +21,7 @@
#include <boost/format.hpp>
#include <boost/functional/factory.hpp>
#include <boost/property_tree/json_parser.hpp>
#include <cstring>

namespace XUtil = XclBinUtilities;

Expand All @@ -37,9 +37,8 @@ SectionAIEResourcesBin::init::init()
{
auto sectionInfo = std::make_unique<SectionInfo>(AIE_RESOURCES_BIN, "AIE_RESOURCES_BIN", boost::factory<SectionAIEResourcesBin*>());
sectionInfo->supportsSubSections = true;
sectionInfo->subSections.push_back(getSubSectionName(SubSection::obj));
sectionInfo->subSections.push_back(getSubSectionName(SubSection::metadata));

sectionInfo->subSections.push_back(std::string(getSubSectionName(SubSection::obj)));
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [hicpp-use-emplace]

Suggested change
sectionInfo->subSections.push_back(std::string(getSubSectionName(SubSection::obj)));
sectionInfo->subSections.emplace_back(getSubSectionName(SubSection::obj));

sectionInfo->subSections.push_back(std::string(getSubSectionName(SubSection::metadata)));
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use emplace_back instead of push_back [hicpp-use-emplace]

Suggested change
sectionInfo->subSections.push_back(std::string(getSubSectionName(SubSection::metadata)));
sectionInfo->subSections.emplace_back(getSubSectionName(SubSection::metadata));

sectionInfo->supportsIndexing = true;

// Add format support empty (no support)
Expand Down Expand Up @@ -166,6 +165,13 @@ SectionAIEResourcesBin::copyBufferUpdateMetadata(const char* _pOrigDataSection,

auto pHdr = reinterpret_cast<const aie_resources_bin*>(_pOrigDataSection);

if (pHdr->mpo_name >= _origSectionSize ||
pHdr->mpo_version >= _origSectionSize ||
pHdr->m_image_offset >= _origSectionSize ||
(pHdr->m_image_offset + pHdr->m_image_size) > _origSectionSize) {
throw std::runtime_error("ERROR: Invalid offsets in aie_resources_bin structure");
}
Comment on lines 166 to +173
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dereferences pHdr->... before proving _origSectionSize >= sizeof(aie_resources_bin). If the section is smaller than the header, simply reading these fields is already an OOB read. Add an early size check (similar to writeObjImage/writeMetadata) before casting/accessing header fields.

Copilot uses AI. Check for mistakes.

XUtil::TRACE_BUF("aie_resources_bin-original", reinterpret_cast<const char*>(pHdr), sizeof(aie_resources_bin));
XUtil::TRACE(boost::format("Original: \n"
" mpo_name (0x%lx): '%s'\n"
Expand Down Expand Up @@ -388,16 +394,19 @@ SectionAIEResourcesBin::writeObjImage(std::ostream& _oStream) const
{
XUtil::TRACE("SectionAIEResourcesBin::writeObjImage");

// Overlay the structure
// Do we have enough room to overlay the header structure
if (m_bufferSize < sizeof(aie_resources_bin)) {
auto errMsg = boost::format("ERROR: Segment size (%d) is smaller than the size of the bmc structure (%d)") % m_bufferSize % sizeof(aie_resources_bin);
throw std::runtime_error(errMsg.str());
}

// No look at the data
auto pHdr = reinterpret_cast<aie_resources_bin*>(m_pBuffer);

if (pHdr->m_image_offset > m_bufferSize ||
pHdr->m_image_size > m_bufferSize ||
pHdr->m_image_offset > m_bufferSize - pHdr->m_image_size) {
throw std::runtime_error("ERROR: Invalid m_image_offset/m_image_size in aie_resources_bin");
}

auto pFWBuffer = reinterpret_cast<const char*>(pHdr) + pHdr->m_image_offset;
_oStream.write(pFWBuffer, pHdr->m_image_size);
}
Expand All @@ -409,38 +418,40 @@ SectionAIEResourcesBin::writeMetadata(std::ostream& _oStream) const
{
XUtil::TRACE("AIE_RESOURCES_BIN-METADATA");

// Overlay the structure
// Do we have enough room to overlay the header structure
if (m_bufferSize < sizeof(aie_resources_bin)) {
auto errMsg = boost::format("ERROR: Segment size (%d) is smaller than the size of the aie_resources_bin structure (%d)") % m_bufferSize % sizeof(aie_resources_bin);
throw std::runtime_error(errMsg.str());
}

auto pHdr = reinterpret_cast<aie_resources_bin*>(m_pBuffer);

auto safe_str = [this, pHdr](uint32_t off) -> std::string {
if (off >= m_bufferSize)
return "";
return std::string(reinterpret_cast<const char*>(pHdr) + off,
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: avoid repeating the return type from the declaration; use a braced initializer list instead [modernize-return-braced-init-list]

    return std::string(reinterpret_cast<const char*>(pHdr) + off,
           ^

strnlen(reinterpret_cast<const char*>(pHdr) + off, m_bufferSize - off));
Comment on lines 427 to +432
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using strnlen(..., m_bufferSize - off) can still allocate/copy up to the remainder of the section if the string is unterminated. With large sections, this can become a memory/time DoS vector. Consider enforcing a reasonable maximum string length cap (e.g., a few KB) in addition to the section bound, and treat longer/unterminated strings as invalid (throw) or truncate intentionally.

Suggested change
auto safe_str = [this, pHdr](uint32_t off) -> std::string {
if (off >= m_bufferSize)
return "";
return std::string(reinterpret_cast<const char*>(pHdr) + off,
strnlen(reinterpret_cast<const char*>(pHdr) + off, m_bufferSize - off));
const size_t kMaxMetadataStringLen = 4096; // Cap individual metadata strings to a reasonable size
auto safe_str = [this, pHdr, kMaxMetadataStringLen](uint32_t off) -> std::string {
if (off >= m_bufferSize)
return "";
const char* ptr = reinterpret_cast<const char*>(pHdr) + off;
size_t remaining = m_bufferSize - off;
size_t maxSearch = (remaining > kMaxMetadataStringLen) ? kMaxMetadataStringLen : remaining;
size_t len = strnlen(ptr, maxSearch);
// If there is more data beyond the maximum allowed length and no terminator was found
if (remaining > kMaxMetadataStringLen && len == maxSearch) {
auto errMsg = boost::format("ERROR: Metadata string at offset 0x%lx exceeds maximum allowed length (%d bytes) or is unterminated")
% static_cast<unsigned long>(off) % kMaxMetadataStringLen;
throw std::runtime_error(errMsg.str());
}
return std::string(ptr, len);

Copilot uses AI. Check for mistakes.
};

XUtil::TRACE(boost::format("Original: \n"
" mpo_name (0x%lx): '%s'\n"
" m_image_offset: 0x%lx, m_image_size: 0x%lx\n"
" mpo_version (0x%lx): '%s'\n"
" m_start_column (0x%lx): '%s'\n"
" m_num_columns (0x%lx): '%s'")
% pHdr->mpo_name % (reinterpret_cast<char*>(pHdr) + pHdr->mpo_name)
% pHdr->mpo_name % safe_str(pHdr->mpo_name)
% pHdr->m_image_offset % pHdr->m_image_size
% pHdr->mpo_version % (reinterpret_cast<char*>(pHdr) + pHdr->mpo_version)
% pHdr->m_start_column % (reinterpret_cast<char*>(pHdr) + pHdr->m_start_column)
% pHdr->m_num_columns % (reinterpret_cast<char*>(pHdr) + pHdr->m_num_columns));
% pHdr->mpo_version % safe_str(pHdr->mpo_version)
% pHdr->m_start_column % safe_str(pHdr->m_start_column)
% pHdr->m_num_columns % safe_str(pHdr->m_num_columns));

// Convert the data from the binary format to JSON
boost::property_tree::ptree ptAieResourcesBin;

ptAieResourcesBin.put("name", reinterpret_cast<char*>(pHdr) + pHdr->mpo_name);
ptAieResourcesBin.put("version", reinterpret_cast<char*>(pHdr) + pHdr->mpo_version);
ptAieResourcesBin.put("start_column", reinterpret_cast<char*>(pHdr) + pHdr->m_start_column);
ptAieResourcesBin.put("num_columns", reinterpret_cast<char*>(pHdr) + pHdr->m_num_columns);
ptAieResourcesBin.put("name", safe_str(pHdr->mpo_name));
ptAieResourcesBin.put("version", safe_str(pHdr->mpo_version));
ptAieResourcesBin.put("start_column", safe_str(pHdr->m_start_column));
ptAieResourcesBin.put("num_columns", safe_str(pHdr->m_num_columns));

boost::property_tree::ptree root;
root.put_child("aie_resources_bin_metadata", ptAieResourcesBin);

boost::property_tree::write_json(_oStream, root);
}

Expand Down
Loading