From 74a07c3f2eb49e0aa67375e64fcc5553475434f3 Mon Sep 17 00:00:00 2001 From: Adam360x Date: Tue, 6 Oct 2026 23:48:49 -0500 Subject: [PATCH 1/2] fix(amdsmi): keep the memory partition name inside the caller's buffer - 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 --- projects/amdsmi/CHANGELOG.md | 3 + projects/amdsmi/rocm_smi/src/rocm_smi.cc | 2 +- .../gpu/partition/memorypartition_read.h | 32 +++++++ .../partition/memorypartition_read_test.cc | 86 +++++++++++++++++++ projects/amdsmi/tests/amd_smi_test/main.cc | 5 ++ projects/rocm-smi-lib/src/rocm_smi.cc | 2 +- 6 files changed, 128 insertions(+), 2 deletions(-) create mode 100644 projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read.h create mode 100644 projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read_test.cc diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index 08a06bdf0109..fab0ff782355 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -21,6 +21,9 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr ### Resolved Issues +- **Fixed a one-byte overrun when reading the memory partition into a small buffer**. + - A buffer too small for the partition name had the byte after its end overwritten. The name is now truncated inside the buffer, and the call still reports that the buffer is too small. + - **Fixed runtime fatal CPERs reporting no AFIDs**. - `amd-smi ras --cper` showed an empty `list afids` column for fatal records, `amd-smi ras --afid --cper-file` printed `-`, and `amdsmi_get_afids_from_cper()` returned no AFIDs. amdgpu writes fatal crashdump sections 32 bytes shorter than `sizeof(cper_sec_crashdump)`, and the section bounds check required the full struct, so every such section was skipped. The check now requires only the dump member the record type uses. diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index 14810367a392..8fd1b48b99fb 100644 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -6784,7 +6784,7 @@ rsmi_status_t rsmi_dev_memory_partition_get(uint32_t dv_ind, char* memory_partit return ret; } - std::size_t buff_size = returning_memory_partition.copy(memory_partition, len); + std::size_t buff_size = returning_memory_partition.copy(memory_partition, len - 1); memory_partition[buff_size] = '\0'; if (len < (returning_memory_partition.size() + 1)) { diff --git a/projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read.h b/projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read.h new file mode 100644 index 000000000000..01b3b377cc66 --- /dev/null +++ b/projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read.h @@ -0,0 +1,32 @@ +// Copyright Advanced Micro Devices, Inc. +// SPDX-License-Identifier: MIT + +#ifndef TESTS_AMD_SMI_TEST_FUNCTIONAL_MEMORYPARTITION_READ_H_ +#define TESTS_AMD_SMI_TEST_FUNCTIONAL_MEMORYPARTITION_READ_H_ + +#include "test_base.h" + +class TestMemoryPartitionRead : public TestBase { + public: + TestMemoryPartitionRead(); + + // @Brief: Destructor for test case of TestMemoryPartitionRead + virtual ~TestMemoryPartitionRead(); + + // @Brief: Setup the environment for measurement + virtual void SetUp(); + + // @Brief: Core measurement execution + virtual void Run(); + + // @Brief: Clean up and retrieve the resource + virtual void Close(); + + // @Brief: Display results + virtual void DisplayResults() const; + + // @Brief: Display information about what this test does + virtual void DisplayTestInfo(void); +}; + +#endif // TESTS_AMD_SMI_TEST_FUNCTIONAL_MEMORYPARTITION_READ_H_ diff --git a/projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read_test.cc b/projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read_test.cc new file mode 100644 index 000000000000..b6a54df7bac2 --- /dev/null +++ b/projects/amdsmi/tests/amd_smi_test/functional/gpu/partition/memorypartition_read_test.cc @@ -0,0 +1,86 @@ +// Copyright Advanced Micro Devices, Inc. +// SPDX-License-Identifier: MIT + +#include "memorypartition_read.h" + +#include + +#include +#include +#include +#include +#include +#include + +#include "amd_smi/amdsmi.h" +#include "test_base.h" +#include "test_common.h" + +TestMemoryPartitionRead::TestMemoryPartitionRead() : TestBase() { + set_title("AMDSMI Memory Partition Read Test"); + set_description( + "Reads the current memory partition into buffers of several sizes and " + "verifies that the result always stays inside the caller's buffer."); +} + +TestMemoryPartitionRead::~TestMemoryPartitionRead(void) {} + +void TestMemoryPartitionRead::SetUp(void) { TestBase::SetUp(); } + +void TestMemoryPartitionRead::DisplayTestInfo(void) { TestBase::DisplayTestInfo(); } + +void TestMemoryPartitionRead::DisplayResults(void) const { TestBase::DisplayResults(); } + +void TestMemoryPartitionRead::Close() { TestBase::Close(); } + +void TestMemoryPartitionRead::Run(void) { + constexpr uint32_t k255Len = 255; + constexpr char kGuard = '#'; + + TestBase::Run(); + PRINT_VERBOSITY(); + if (setup_failed_) { + std::cout << "** SetUp Failed for this test. Skipping.**" << std::endl; + return; + } + + for (uint32_t dv_ind = 0; dv_ind < num_monitor_devs(); ++dv_ind) { + PrintDeviceHeader(processor_handles_[dv_ind]); + + char full[k255Len] = {}; + amdsmi_status_t err = + amdsmi_get_gpu_memory_partition(processor_handles_[dv_ind], full, k255Len); + if (err == AMDSMI_STATUS_NOT_SUPPORTED) { + IF_VERB(STANDARD) { + std::cout << "\t**Memory partition is not supported on this device" << std::endl; + } + continue; + } + ASSERT_EQ(err, AMDSMI_STATUS_SUCCESS); + const std::string name(full); + if (name.empty()) { + IF_VERB(STANDARD) { + std::cout << "\t**Memory partition is empty on this device" << std::endl; + } + continue; + } + IF_VERB(STANDARD) { std::cout << "\t**Current memory partition: " << name << std::endl; } + + // Each buffer has `len` usable bytes followed by one guard byte. A buffer + // too small for the name must hold a terminated prefix of it, and a buffer + // that fits must hold all of it. The guard byte must stay untouched. + const auto name_len = static_cast(name.size()); + for (uint32_t len : {1u, 2u, name_len, name_len + 1}) { + std::vector buf(len + 1, kGuard); + err = amdsmi_get_gpu_memory_partition(processor_handles_[dv_ind], buf.data(), len); + const bool fits = len > name_len; + EXPECT_EQ(err, fits ? AMDSMI_STATUS_SUCCESS : AMDSMI_STATUS_INSUFFICIENT_SIZE) + << "len=" << len; + EXPECT_EQ(buf[len], kGuard) << "wrote past the end of a " << len << "-byte buffer"; + const size_t used = strnlen(buf.data(), len); + EXPECT_LT(used, len) << "no terminator inside a " << len << "-byte buffer"; + EXPECT_EQ(std::string(buf.data(), used), fits ? name : name.substr(0, len - 1)) + << "len=" << len; + } + } +} diff --git a/projects/amdsmi/tests/amd_smi_test/main.cc b/projects/amdsmi/tests/amd_smi_test/main.cc index a3757934f4c3..40c53f223322 100644 --- a/projects/amdsmi/tests/amd_smi_test/main.cc +++ b/projects/amdsmi/tests/amd_smi_test/main.cc @@ -26,6 +26,7 @@ #include "functional/gpu/metrics/process_list_read.h" #include "functional/gpu/partition/computepartition_memallocmode_read_write.h" #include "functional/gpu/partition/computepartition_read_write.h" +#include "functional/gpu/partition/memorypartition_read.h" #include "functional/gpu/partition/memorypartition_read_write.h" #include "functional/gpu/pci/pci_read_write.h" #include "functional/gpu/perf/overdrive_read.h" @@ -248,6 +249,10 @@ TEST(GpuFunctionalReadOnly, TestGpuPartitionMetricsRead) { TestGpuPartitionMetricsRead tst; RunGenericTest(&tst); } +TEST(GpuFunctionalReadOnly, TestMemoryPartitionRead) { + TestMemoryPartitionRead tst; + RunGenericTest(&tst); +} TEST(GpuFunctionalReadOnly, TestMetricsCounterRead) { TestMetricsCounterRead tst; RunGenericTest(&tst); diff --git a/projects/rocm-smi-lib/src/rocm_smi.cc b/projects/rocm-smi-lib/src/rocm_smi.cc index acabbff005ed..fa6537241542 100644 --- a/projects/rocm-smi-lib/src/rocm_smi.cc +++ b/projects/rocm-smi-lib/src/rocm_smi.cc @@ -5119,7 +5119,7 @@ rsmi_status_t rsmi_dev_memory_partition_get(uint32_t dv_ind, char* memory_partit return ret; } - std::size_t buff_size = returning_memory_partition.copy(memory_partition, len); + std::size_t buff_size = returning_memory_partition.copy(memory_partition, len - 1); memory_partition[buff_size] = '\0'; if (len < (returning_memory_partition.size() + 1)) { From 13511535ceaea2b8ea5c17c1fcd0c0f6c89a3ccc Mon Sep 17 00:00:00 2001 From: Adam360x Date: Wed, 7 Oct 2026 01:40:40 -0500 Subject: [PATCH 2/2] test(rocm-smi-lib): guard the memory partition small-buffer check - 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 --- .../functional/memorypartition_read_write.cc | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/projects/rocm-smi-lib/tests/rocm_smi_test/functional/memorypartition_read_write.cc b/projects/rocm-smi-lib/tests/rocm_smi_test/functional/memorypartition_read_write.cc index b6ee80e26d33..74535bdeaa8b 100644 --- a/projects/rocm-smi-lib/tests/rocm_smi_test/functional/memorypartition_read_write.cc +++ b/projects/rocm-smi-lib/tests/rocm_smi_test/functional/memorypartition_read_write.cc @@ -191,18 +191,22 @@ void TestMemoryPartitionReadWrite::Run(void) { } ASSERT_TRUE(ret == RSMI_STATUS_SUCCESS); - // Verify api support checking functionality is working - uint32_t kLen = 2; - char smallBuffer[kLen]; + // Verify api support checking functionality is working. The byte after + // the kLen usable bytes is a guard: the name must be truncated and + // terminated inside kLen, with nothing written past it. + constexpr uint32_t kLen = 2; + constexpr char kGuard = '#'; + char smallBuffer[kLen + 1]; + smallBuffer[kLen] = kGuard; err = rsmi_dev_memory_partition_get(dv_ind, smallBuffer, kLen); - size_t size = sizeof(smallBuffer) / sizeof(*smallBuffer); ASSERT_EQ(err, RSMI_STATUS_INSUFFICIENT_SIZE); - ASSERT_EQ((size_t)kLen, size); + ASSERT_EQ(smallBuffer[kLen], kGuard); + ASSERT_EQ(smallBuffer[kLen - 1], '\0'); if (err == RSMI_STATUS_INSUFFICIENT_SIZE) { IF_VERB(STANDARD) { std::cout << "\t**" << "Confirmed RSMI_STATUS_INSUFFICIENT_SIZE was returned " - << "and size matches kLen requested." << std::endl; + << "and nothing was written past kLen." << std::endl; } }