Adding OOB checks in the xclbin_parser

[ManojTakasi]

Problem solved by the commit

https://amd.atlassian.net/wiki/spaces/~saifuddi/pages/1407497203/XRT+UserSpace+Bug+Bounty
SWSPLAT-9168: OOB read in xclbin SOFT_KERNEL parsing via unvalidated mpo_* offsets, crafted xclbins could cause crash or info leak.
SWSPLAT-9114 / 9170 / 9154: OOB read and integer overflow in AIE_PARTITION parsing (get_aie_partition), file-controlled offsets/sizes used without bounds checks, leading to crash or metadata corruption.

SWSPLAT-9150: Use-after-free and OOB read in xclbinutil SectionAIEResourcesBin (init, copyBufferUpdateMetadata, writeObjImage, writeMetadata) when processing malformed AIE_RESOURCES_BIN sections.

SWSPLAT-9110: Command injection in xball via prog_args concatenated into a shell command passed to os.system().

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

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

Added section bounds checks and overflow checks and use strnlen for all string fields from section data.
Added offset/size validation and bounded string reads and store subsection names by value to avoid UAF
Run command via subprocess.run(cmd_list) with shlex.split(prog_args) instead of os.system(cmd).

Risks (if any) associated the changes in the commit

low

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

Tested by building XRT package on ubuntu 22.04

Documentation impact (if any)

[Copilot]

Pull request overview

Adds defensive parsing and execution hardening to address multiple security issues in xclbin parsing and tooling (OOB reads/UAF/overflow, plus command injection).

Changes:

• Added bounds/overflow checks and bounded string reads (via strnlen) in xclbin/AIE section parsing.
• Avoided potential UAF by storing subsection names by value in SectionAIEResourcesBin.
• Replaced os.system() with subprocess.run() + shlex.split() in xball to prevent command injection.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File	Description
src/runtime_src/tools/xclbinutil/SectionAIEResourcesBin.cxx	Adds offset validation and bounded string extraction for AIE_RESOURCES_BIN processing.
src/runtime_src/core/tools/common/xball	Switches to argv-style subprocess execution to avoid shell command injection.
src/runtime_src/core/common/xclbin_parser.cpp	Adds bounds checks, overflow-resistant parsing, and bounded string reads for SOFT_KERNEL and AIE_PARTITION sections.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[github-actions]

clang-tidy made some suggestions

[github-actions]

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));

[github-actions]

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));

[github-actions]

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,
           ^