CodeQL Fix OOB reads in SectionAIEResourcesBin - #9961
Closed
stsoe wants to merge 1 commit into
Closed
Conversation
#### Problem solved by the commit Four CWE-125 heap OOB reads in copyBufferUpdateMetadata() where aie_resources_bin mpo_* fields (mpo_name, mpo_version, m_start_column, m_num_columns) were used as raw pointer offsets without bounds checking when computing sDefault fallback strings, bypassing the bounded_mpo_cstr() guard added in bb1791f. Two additional CWE-125 OOB reads where m_image_offset and m_image_size were used without bounds checking in both copyBufferUpdateMetadata() and writeObjImage() before reading image data. #### Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered Jira tickets: - AIESW-41124 - AIESW-41125 - AIESW-41126 - AIESW-41127 - AIESW-41128 - AIESW-41129 CodeQL alerts Xilinx#135-Xilinx#140 (amd-psirt/xclbin-parser-oob, HIGH severity). The sDefault reads were missed by bb1791f (SWSPLAT-30717), which protected the TRACE() and writeMetadata() paths but not the fallback default value paths in copyBufferUpdateMetadata(). The image offset/size checks were never added to writeObjImage(). #### How problem was solved, alternative solutions (if any) and why they were rejected Replaced the four raw sDefault pointer calculations with bounded_mpo_cstr(pHdr, pHdr->field, _origSectionSize). This also fixes a pre-existing double-offset bug in the original code: the sDefault expressions incorrectly added sizeof(aie_resources_bin) on top of the mpo_* field value, but mpo_* fields already store absolute offsets from pHdr (sizeof is baked in at write time), so the fallback pointer was pointing past the actual string. Added explicit uint64_t overflow-safe bounds checks for m_image_offset + m_image_size against section size in both copyBufferUpdateMetadata() and writeObjImage(). #### Risks (if any) associated the changes in the commit Low. The sDefault path is only taken when the JSON metadata stream omits a key; the double-offset bug meant the fallback was producing wrong values anyway. The image bounds checks only reject malformed xclbins. #### What has been tested and how, request additional testing if necessary Built xclbinutil successfully. Recommend testing xclbinutil --input with a crafted xclbin containing OOB mpo offsets and verifying it throws rather than reading past the buffer. #### Documentation impact (if any) None Signed-off-by: Soren Soe <2106410+stsoe@users.noreply.github.com> Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
clang-tidy review says "All clean, LGTM! 👍" |
xfreid
approved these changes
Aug 5, 2026
Collaborator
Author
|
Replaced by #9966 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem solved by the commit
Four CWE-125 heap OOB reads in copyBufferUpdateMetadata() where aie_resources_bin mpo_* fields (mpo_name, mpo_version, m_start_column, m_num_columns) were used as raw pointer offsets without bounds checking when computing sDefault fallback strings, bypassing the bounded_mpo_cstr() guard added in bb1791f.
Two additional CWE-125 OOB reads where m_image_offset and m_image_size were used without bounds checking in both copyBufferUpdateMetadata() and writeObjImage() before reading image data.
Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered
Jira tickets:
CodeQL alerts #135-#140 (amd-psirt/xclbin-parser-oob, HIGH severity). The sDefault reads were missed by bb1791f (SWSPLAT-30717), which protected the TRACE() and writeMetadata() paths but not the fallback default value paths in copyBufferUpdateMetadata(). The image offset/size checks were never added to writeObjImage().
How problem was solved, alternative solutions (if any) and why they were rejected
Replaced the four raw sDefault pointer calculations with bounded_mpo_cstr(pHdr, pHdr->field, origSectionSize). This also fixes a pre-existing double-offset bug in the original code: the sDefault expressions incorrectly added sizeof(aie_resources_bin) on top of the mpo* field value, but mpo_* fields already store absolute offsets from pHdr (sizeof is baked in at write time), so the fallback pointer was pointing past the actual string.
Added explicit uint64_t overflow-safe bounds checks for m_image_offset + m_image_size against section size in both copyBufferUpdateMetadata() and writeObjImage().
Risks (if any) associated the changes in the commit
Low. The sDefault path is only taken when the JSON metadata stream omits a key; the double-offset bug meant the fallback was producing wrong values anyway. The image bounds checks only reject malformed xclbins.
What has been tested and how, request additional testing if necessary
Built xclbinutil successfully. Recommend testing xclbinutil --input with a crafted xclbin containing OOB mpo offsets and verifying it throws rather than reading past the buffer.
Documentation impact (if any)
None