Repository navigation
fix(amdsmi): keep the memory partition name inside the caller's buffer - #12986
Merged
Merged
Conversation
- Copy at most len - 1 characters before the terminator, as the compute partition getter already does. A buffer too small for the name had its terminator written one byte past its end - Apply the same one-line fix to the rocm-smi-lib copy of the function - Add a read-only test that reads the partition into short, exact-fit and larger buffers with a guard byte after each. It needs no root and skips GPUs without memory partitions - Add a CHANGELOG entry Signed-off-by: Adam360x <Adam.pryor@amd.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The independently maintained rocm-smi-lib implementation lacks regression coverage capable of detecting the fixed overflow.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes memory-partition reads so truncated names remain null-terminated within caller-provided buffers.
Changes:
- Limits copies to
len - 1in both library implementations. - Adds guard-byte regression coverage for amd-smi.
- Documents the resolved overflow.
| File | Description |
|---|---|
projects/rocm-smi-lib/src/rocm_smi.cc |
Fixes bounded memory-partition copying. |
projects/amdsmi/rocm_smi/src/rocm_smi.cc |
Applies the same fix to amd-smi’s bundled implementation. |
projects/amdsmi/tests/amd_smi_test/main.cc |
Registers the read-only regression test. |
projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read.h |
Declares the new test fixture. |
projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read_test.cc |
Tests truncation, termination, status, and guard preservation. |
projects/amdsmi/CHANGELOG.md |
Records the resolved overflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Add a guard byte after the 2-byte buffer and check that the name is terminated inside it, so the rocm-smi-lib test fails if the copy writes past the caller's buffer again - Drop the size check, which compared the buffer with itself Signed-off-by: Adam360x <Adam.pryor@amd.com>
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
…M-30433 # Conflicts: # projects/amdsmi/CHANGELOG.md Co-authored-by: adam360x <61172547+adam360x@users.noreply.github.com>
Contributor
bill-shuzhou-liu
approved these changes
Oct 8, 2026
gabrpham
approved these changes
Oct 8, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
anujshuk-amd
pushed a commit
that referenced
this pull request
Oct 12, 2026
#12986) ## Motivation - TheRock's ASAN nightly aborts `amdsmitst` in `GpuFunctionalReadWrite.TestMemoryPartitionReadWrite` with a stack-buffer-overflow (MI325X). The test passes a 2-byte buffer to `amdsmi_get_gpu_memory_partition()` and expects "insufficient size". - The bug is in the library, not the test: `rsmi_dev_memory_partition_get()` copied up to `len` characters and then wrote the terminator after them, so any buffer of `len` bytes or less got one byte written past its end. - Present since 2023 in both copies of the function: amd-smi's bundled rocm_smi and rocm-smi-lib. Signed-off-by: Adam360x <Adam.pryor@amd.com>
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
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.
Motivation
amdsmitstinGpuFunctionalReadWrite.TestMemoryPartitionReadWritewith a stack-buffer-overflow (MI325X). The test passes a 2-byte buffer toamdsmi_get_gpu_memory_partition()and expects "insufficient size".rsmi_dev_memory_partition_get()copied up tolencharacters and then wrote the terminator after them, so any buffer oflenbytes or less got one byte written past its end.Technical Details
rocm_smi.ccin amd-smi and in rocm-smi-lib: copy at mostlen - 1characters, as the compute-partition getter already does. Status codes are unchanged; a short buffer still gets "insufficient size", now with a truncated, terminated name.GpuFunctionalReadOnly.TestMemoryPartitionReadIssue Tracking
JIRA ID: ROCM-30433
Test Plan
len2), BEFORE and AFTER.GpuFunctionalReadOnlytests on the MI308X box, BEFORE and AFTER.amdsmitstlocally (gfx1030 + gfx1100, no memory partitions), Release and ASAN with the AMDSMI ASAN workflow's options.rsmitstbuild.Test Result
stack-buffer-overflow,WRITE of size 1,'smallBuffer' ... overflows this variable, atrocm_smi.cc:6788 in rsmi_dev_memory_partition_get(the report from the issue). AFTER: no report; "insufficient size" with buffer"N".Submission Checklist