Security review of xclbinutil

[stsoe]

Problem solved by the commit

A thorough manual security review of src/runtime_src/tools/xclbinutil identified 17 vulnerabilities across 10 files where wire-format fields from attacker-controlled xclbin files were used without adequate bounds checking. The fixes are grouped into six categories below.

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

All bugs are pre-existing. Discovered by manual security review following CodeQL alert triage (alerts #129–#156). None of the bugs below were previously flagged by CodeQL.

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

Category 1 — CWE-125: Unterminated fixed-size string fields (5 bugs)

Five parsers passed fixed-size embedded char/uint8_t arrays from wire-format structs directly to boost::format("%s"), which calls strlen and reads to the first null byte. A crafted xclbin with no null terminator in the last entry's string field causes a heap OOB read past the section buffer.

Affected: ip_data::m_name (SectionIPLayout), clock_freq::m_name (SectionClockFrequencyTopology), mem_data::m_tag (SectionMemTopology, SectionGroupTopology), debug_ip_data::m_name (SectionDebugIPLayout).

Fix: Added XclBinUtilities::bounded_fixed_cstr<N>() template that uses strnlen to bound the read to the field width. Applied at all put() and TRACE call sites in all five parsers.

---

Category 2 — CWE-125: Unchecked wire-format array offsets in SectionAIEPartition (7 bugs)

writeAIEPartitionImage() had bufferSize available but never passed it to any helper function, leaving all wire-format offset/size pointer arithmetic unguarded. Seven sites were affected including one where attacker-controlled data was written to a PDI output file on disk (information disclosure).

Affected fields: aie_pdi.offset/size, start_columns.offset/size, cdo_groups.offset/size, cdo_group::mpo_name, dpu_kernel_ids.offset/size, pre_cdo_groups.offset/size, pdi_image.offset/size.

Fix: Added bufferSize parameter to all helper functions. Added overflow-safe bounds checks (count > (bufferSize - offset) / sizeof(T)) before every array pointer formation. Replaced raw mpo_name pointer arithmetic with bounded_mpo_cstr(). Bounds-check temporaries reused in array cast and loop bounds.

---

Category 3 — CWE-190/191: Integer overflow and underflow (4 bugs)

• SectionMCS.cxx: m_chunk::m_offset (uint64_t) was added to pointer before the bounds check. Pointer wraparound on x86-64 caused the check to pass for values near UINT64_MAX. Fix: validate offset and size directly in offset-space before forming the pointer.

• SectionDNACertificate.cxx: paddingSize computed as (sectionSize - signatureSizeBytes) - paddingOffset underflows to ~2^64 when paddingOffset is large, because the guard did not account for the signature and length-field overhead. Fix: include overhead in the guard; add explicit underflow check before subtraction.

• XclBinUtilities.cxx (seekg): signatureOffset + signedByOffset computed as unsigned int before widening to std::streamoff, wrapping the seek position to zero. Fix: cast to uint64_t before adding. Also added missing gcount() checks on all three signature reads.

• XclBinUtilities.cxx (MBG loop): inner while accessed memIndexVector[idx + 1] with no idx + 1 < size guard, causing a one-past-the-end read on the last iteration. Fix: add bounds guard to the while condition.

---

Category 4 — CWE-190: Dead guard + silent integer truncation in Section.cxx (1 bug)

The guard m_sectionSize > UINT64_MAX is tautologically false for a uint64_t. The subsequent cast to unsigned int silently truncated any section size above 4 GiB, allocating a tiny buffer while all downstream consumers saw the wrong size with no error. Same dead-code pattern in two locations. Fix: replace > UINT64_MAX with > UINT32_MAX.

---

Category 5 — Logic/robustness bugs (2 bugs)

• XclBinClass.cxx: ptInterfaces[0] was accessed unconditionally without checking whether the vector was empty, causing undefined behavior (crash) for xclbins with no interfaces node. Fix: add if (ptInterfaces.empty()) return; guard.

• XclBinUtilities.cxx: istream::read() of the SignatureHeader struct had no gcount() check. A truncated file left the struct zero-initialized, silently skipping all data reads. Fix: add gcount() check consistent with the existing checks on the data reads.

---

Category 6 — CWE-78: Command injection in legacy popen path (1 bug)

The Boost < 1.64 fallback in exec() joined cmd and args with spaces and passed the result to popen(). A user-controlled argument (e.g. --add-pskernel path) containing shell metacharacters could inject arbitrary shell commands. The Boost >= 1.64 path using boost::process::child is not affected. Fix: added shell_quote() helper that wraps each argument in single quotes and escapes embedded single quotes as '"'"', preventing shell interpretation of any argument content.

Risks (if any) associated the changes in the commit

Low across all categories. All changes are input validation only; behavior for well-formed xclbin files is unchanged. Malformed files that previously caused silent heap reads, data corruption, or crashes now throw std::runtime_error, which xclbinutil already handles at its top level.

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

Built xclbinutil successfully for all six change sets. Recommend:

• Fuzzing with crafted xclbins containing out-of-bounds mpo_* offsets and array counts, and verifying xclbinutil throws rather than leaking heap data.
• Testing --add-pskernel with a path containing shell metacharacters on a Boost < 1.64 build and verifying no injection occurs.
• AddressSanitizer build to confirm no residual OOB reads remain.

Documentation impact (if any)

None.