From bbdec866e4ebcdc01ccec376c34e8960623d4599 Mon Sep 17 00:00:00 2001 From: Soren Soe <2106410+stsoe@users.noreply.github.com> Date: Tue, 30 Jun 2026 14:49:32 -0700 Subject: [PATCH] SWSPLAT-30704 firmware_image fix security issues in xsabin parsing Six security defects were found and fixed in the xsabin/dsabin parsing code that handles firmware images supplied by users or loaded from disk. 1. remove_xsabin_mirror: broken mirror-tag search The original code cast the buffer pointer to a stringstream and called find() on it, which searched the pointer's textual address rather than the buffer contents. Offsets derived this way were completely wrong and the subsequent memset/memcpy had no bounds checks. Fixed by using std::search over the raw byte range; offsets are now derived from pointer arithmetic guaranteed to be within [0, bufferSize). 2. remove_xsabin_section: integer underflow in header-shift memcpy bytesToShift was computed as m_length minus the byte distance to the next section header. If that distance exceeded m_length (e.g. from a crafted xsabin), the subtraction underflowed to a huge value, causing a wild memcpy. Fixed by checking fromOffset against m_length before subtracting. 3. Section offset/size not validated against buffer length m_sectionOffset and m_sectionSize from the axlf header were used directly to allocate buffers and seek/read from the file with no check that [offset, offset+size) fits within m_length. A crafted binary could trigger out-of-bounds reads or huge allocations. Fixed with a new check_section_bounds() helper (overflow-safe: guards size > buf_len - offset) applied at all five section-read sites (DTB, BMC x2, FLASH, MCS). 4. Embedded struct fields not validated against section bounds Fields bmc->m_offset/m_size, flashMeta.m_image_offset/m_image_size, and mcs_chunk::m_offset/m_size come from within the file and were used to compute seek offsets and allocation sizes with no range check against the enclosing section. Also, no check that the section is large enough to hold the header struct itself. Fixed by applying check_section_bounds() at each embedded-offset site and adding sizeof(struct) minimum-size guards. 5. matchId: out-of-bounds access due to inverted empty check The condition `if (uuids.empty())` guarded an access to uuids[0], meaning the access was reached only when the vector was empty, causing undefined behaviour. Fixed by inverting to !uuids.empty(). 6. SWSPLAT-30704: remove_xsabin_section() heap OOB write via attacker-controlled section offsets Co-Authored-By: Claude Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com> --- .../tools/xbmgmt2/flash/firmware_image.cpp | 137 ++++++++++++------ 1 file changed, 93 insertions(+), 44 deletions(-) diff --git a/src/runtime_src/core/tools/xbmgmt2/flash/firmware_image.cpp b/src/runtime_src/core/tools/xbmgmt2/flash/firmware_image.cpp index 47eace5cc2d..7cd84f5f79b 100644 --- a/src/runtime_src/core/tools/xbmgmt2/flash/firmware_image.cpp +++ b/src/runtime_src/core/tools/xbmgmt2/flash/firmware_image.cpp @@ -1,38 +1,23 @@ -/** - * Copyright (C) 2019-2022 Xilinx, Inc - * Copyright (C) 2022 Advanced Micro Devices, Inc. - * - * 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 - * License is located at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT - * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the - * License for the specific language governing permissions and limitations - * under the License. - */ - +// SPDX-License-Identifier: Apache-2.0 +// Copyright (C) 2019-2022 Xilinx, Inc +// Copyright (C) 2022-2026 Advanced Micro Devices, Inc. - All rights reserved #include "firmware_image.h" #include "core/common/utils.h" #include "core/include/xrt/detail/xclbin.h" -// 3rd Party Library - Include Files #include #include #include -#include +#include +#include +#include +#include #include #include -#include -#include #include +#include #include -#include -#include #include #ifdef _WIN32 @@ -175,6 +160,16 @@ void getUUIDFromDTB(void *blob, uint64_t &ts, std::vector &uuids) uuid2ts(uuids[0], ts); } +// Verify that [offset, offset+size) fits within a buffer of known length. +// Throws on overflow or out-of-range access. +static void +check_section_bounds(uint64_t offset, uint64_t size, uint64_t buf_len, + const char *ctx) +{ + if (offset > buf_len || size > buf_len - offset) + throw std::runtime_error(std::string(ctx) + ": section offset/size out of range"); +} + DSAInfo::DSAInfo(const std::string& filename, uint64_t ts, const std::string& id, const std::string& bmcV) : hasFlashImage(false), vendor(), board(), name(), file(filename), timestamp(ts), bmcVer(bmcV), @@ -301,6 +296,8 @@ DSAInfo::DSAInfo(const std::string& filename, uint64_t ts, const std::string& id // Show it as ID for flashing const axlf_section_header* dtbSection = xclbin::get_axlf_section(ap, PARTITION_METADATA); if (dtbSection && timestamp == NULL_TIMESTAMP) { + check_section_bounds(dtbSection->m_sectionOffset, dtbSection->m_sectionSize, + ap->m_header.m_length, "DTB section"); std::vector dtbbuf(dtbSection->m_sectionSize); in.seekg(dtbSection->m_sectionOffset); in.read(dtbbuf.data(), dtbSection->m_sectionSize); @@ -317,6 +314,8 @@ DSAInfo::DSAInfo(const std::string& filename, uint64_t ts, const std::string& id if (bmcSection == nullptr) return; // Load entire BMC section. + check_section_bounds(bmcSection->m_sectionOffset, bmcSection->m_sectionSize, + ap->m_header.m_length, "BMC section (DSAInfo)"); std::vector bmcbuf(bmcSection->m_sectionSize); in.seekg(bmcSection->m_sectionOffset); in.read(bmcbuf.data(), bmcSection->m_sectionSize); @@ -367,7 +366,7 @@ bool DSAInfo::matchId(const std::string &id) const if (ts != 0 && ts != ULLONG_MAX && ts == timestamp) return true; - if (uuids.empty()) { + if (!uuids.empty()) { const std::string uuid = normalize_uuid(id); if (!strncmp(uuids[0].c_str(), uuid.c_str(), uuid.length())) @@ -469,29 +468,36 @@ remove_xsabin_mirror(void * xsabin_buffer) axlf *axlf_header = reinterpret_cast(xsabin_buffer); uint64_t bufferSize = axlf_header->m_header.m_length; - std::stringstream strm; - strm << xsabin_buffer; - std::string str_xsabin_buffer = strm.str(); + auto *buf_begin = reinterpret_cast(xsabin_buffer); + auto *buf_end = buf_begin + bufferSize; - auto start_offset = str_xsabin_buffer.find(mirror_data_start); - if (start_offset == std::string::npos) + auto *start_ptr = std::search(buf_begin, buf_end, + mirror_data_start.begin(), mirror_data_start.end()); + if (start_ptr == buf_end) return; // No MIRROR DATA - auto end_offset = str_xsabin_buffer.find(mirror_data_end); - if (end_offset == std::string::npos) + auto *end_ptr = std::search(buf_begin, buf_end, + mirror_data_end.begin(), mirror_data_end.end()); + if (end_ptr == buf_end) return; // Badly formed mirror data (we have a start, but no end) - if (end_offset <= start_offset) - return; // Tags are in the wrong order + if (end_ptr <= start_ptr) + return; // Tags are in the wrong order - // Zero out memory (not really needed but done for completeness) + uint64_t start_offset = static_cast(start_ptr - buf_begin); + uint64_t end_offset = static_cast(end_ptr - buf_begin); + + // Both offsets are within [0, bufferSize) by construction (search returned + // pointers inside [buf_begin, buf_end)), so no further range check needed. uint64_t bytesRemoved = end_offset - start_offset; - std::memset(reinterpret_cast(xsabin_buffer) + start_offset, 0, bytesRemoved); + uint64_t bytesToCopy = bufferSize - end_offset; + + // Zero out memory (not really needed but done for completeness) + std::memset(buf_begin + start_offset, 0, bytesRemoved); // Compress the image - uint64_t bytesToCopy = bufferSize - end_offset; if (bytesToCopy != 0) - std::memcpy(reinterpret_cast(xsabin_buffer) + start_offset, reinterpret_cast(xsabin_buffer) + end_offset, bytesToCopy); + std::memcpy(buf_begin + start_offset, buf_begin + end_offset, bytesToCopy); // Update length of the buffer axlf_header->m_header.m_length = bufferSize - bytesRemoved; @@ -514,20 +520,27 @@ remove_xsabin_section(void * xsabin_buffer, enum axlf_section_kind section_to_re // Is this a section of interest, if not then go to the next section if (sectionHeaderArray[index].m_sectionKind != static_cast(section_to_remove)) continue; + // Record the buffer size uint64_t bufferSize = axlf_header->m_header.m_length; // Determine the data to be removed. uint64_t startToOffset = sectionHeaderArray[index].m_sectionOffset; - uint64_t startFromOffset = ((index + 1) == axlf_header->m_header.m_numSections) ? - sectionHeaderArray[index].m_sectionOffset + sectionHeaderArray[index].m_sectionSize: - sectionHeaderArray[index + 1].m_sectionOffset; + uint64_t startFromOffset = ((index + 1) == axlf_header->m_header.m_numSections) + ? sectionHeaderArray[index].m_sectionOffset + sectionHeaderArray[index].m_sectionSize + : sectionHeaderArray[index + 1].m_sectionOffset; + + if (startToOffset > bufferSize + || startFromOffset < startToOffset + || startFromOffset > bufferSize) + throw std::runtime_error("xsabin: section offset/size out of range"); + uint64_t bytesToCopy = bufferSize - startFromOffset; uint64_t bytesRemoved = startFromOffset - startToOffset; - if (bytesToCopy != 0) { + if (bytesToCopy) { std::memcpy(reinterpret_cast(xsabin_buffer) + startToOffset, - reinterpret_cast(xsabin_buffer) + startFromOffset, bytesToCopy); + reinterpret_cast(xsabin_buffer) + startFromOffset, bytesToCopy); } // -- Now do some incremental clean up of the data structures // Update the length and offsets AFTER this entry @@ -546,8 +559,14 @@ remove_xsabin_section(void * xsabin_buffer, enum axlf_section_kind section_to_re // Remove the array entry void * ptrStartTo = §ionHeaderArray[index]; void * ptrStartFrom = §ionHeaderArray[index+1]; - uint64_t bytesToShift = axlf_header->m_header.m_length - - (reinterpret_cast(ptrStartFrom) - reinterpret_cast(xsabin_buffer)); + auto fromOffset = static_cast( + reinterpret_cast(ptrStartFrom) - + reinterpret_cast(xsabin_buffer)); + + if (fromOffset > axlf_header->m_header.m_length) + throw std::runtime_error("xsabin: section header array extends beyond buffer"); + + uint64_t bytesToShift = axlf_header->m_header.m_length - fromOffset; std::memcpy(reinterpret_cast(ptrStartTo), reinterpret_cast(ptrStartFrom), bytesToShift); // Update data elements @@ -616,6 +635,13 @@ firmwareImage::firmwareImage(const std::string& file, imageType type) : return; } // Load entire BMC section. + check_section_bounds(bmcSection->m_sectionOffset, bmcSection->m_sectionSize, + ap->m_header.m_length, "BMC section"); + if (bmcSection->m_sectionSize < sizeof(struct bmc)) { + this->setstate(failbit); + std::cout << "BMC section too small in "<< file << std::endl; + return; + } std::vector bmcbuf(bmcSection->m_sectionSize); in_file.seekg(bmcSection->m_sectionOffset); in_file.read(bmcbuf.data(), bmcSection->m_sectionSize); @@ -626,6 +652,9 @@ firmwareImage::firmwareImage(const std::string& file, imageType type) : return; } const struct bmc *bmc = reinterpret_cast(bmcbuf.data()); + // Validate embedded offset/size against the section bounds. + check_section_bounds(bmc->m_offset, bmc->m_size, + bmcSection->m_sectionSize, "BMC image within section"); // Load data into stream. bufsize = bmc->m_size; mBuf = new char[bufsize]; @@ -674,6 +703,13 @@ firmwareImage::firmwareImage(const std::string& file, imageType type) : } //load 'struct flash' + check_section_bounds(flashSection->m_sectionOffset, flashSection->m_sectionSize, + ap->m_header.m_length, "FLASH section"); + if (flashSection->m_sectionSize < sizeof(struct flash)) { + this->setstate(failbit); + std::cout << "FLASH section too small in "<< file << std::endl; + return; + } struct flash flashMeta; in_file.seekg(flashSection->m_sectionOffset); in_file.read(reinterpret_cast(&flashMeta), sizeof(flashMeta)); @@ -683,6 +719,9 @@ firmwareImage::firmwareImage(const std::string& file, imageType type) : std::cout << "Can't read FLASH section from "<< file << std::endl; return; } + // Validate embedded image offset/size against the section bounds. + check_section_bounds(flashMeta.m_image_offset, flashMeta.m_image_size, + flashSection->m_sectionSize, "flash image within section"); // Load data into stream. bufsize = flashMeta.m_image_size; mBuf = new char[bufsize]; @@ -716,6 +755,13 @@ firmwareImage::firmwareImage(const std::string& file, imageType type) : return; } // Load entire MCS section. + check_section_bounds(mcsSection->m_sectionOffset, mcsSection->m_sectionSize, + ap->m_header.m_length, "MCS section"); + if (mcsSection->m_sectionSize < sizeof(struct mcs)) { + this->setstate(failbit); + std::cout << "MCS section too small in "<< file << std::endl; + return; + } std::vector mcsbuf(mcsSection->m_sectionSize); in_file.seekg(mcsSection->m_sectionOffset); in_file.read(mcsbuf.data(), mcsSection->m_sectionSize); @@ -742,6 +788,9 @@ firmwareImage::firmwareImage(const std::string& file, imageType type) : this->setstate(failbit); return; } + // Validate embedded chunk offset/size against the section bounds. + check_section_bounds(c->m_offset, c->m_size, + mcsSection->m_sectionSize, "MCS chunk within section"); // Load data into stream. bufsize = c->m_size; mBuf = new char[bufsize];