hw/i2c/pmbus_device.c | 13 +++++++++++++ hw/sensor/adm1266.c | 7 +++---- include/hw/i2c/pmbus_device.h | 10 ++++++++++ 3 files changed, 26 insertions(+), 4 deletions(-)
The PMBus string registers (e.g. MFR_MODEL on the ADM1266) are writable
by the guest. pmbus_receive_block() can fill the destination field
completely, leaving no NUL terminator. When the value is later read back,
pmbus_send_string() calls strlen() on it, which reads past the end of the
array and returns a length that trips
g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
aborting QEMU. This is guest-triggerable.
Add pmbus_receive_string(), the write-side mirror of pmbus_send_string(),
which reserves the last byte of the destination so the stored value is
always NUL-terminated, and use it for the ADM1266 MFR string registers.
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3388
Signed-off-by: imaginos <imaginos32@gmail.com>
---
hw/i2c/pmbus_device.c | 13 +++++++++++++
hw/sensor/adm1266.c | 7 +++----
include/hw/i2c/pmbus_device.h | 10 ++++++++++
3 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index b1f9843f52..7c65d8c8b5 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -142,6 +142,19 @@ uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len)
return len;
}
+uint8_t pmbus_receive_string(PMBusDevice *pmdev, char *dest, size_t len)
+{
+ uint8_t rv;
+
+ if (len == 0) {
+ return 0;
+ }
+
+ rv = pmbus_receive_block(pmdev, (uint8_t *)dest, len - 1);
+ dest[len - 1] = '\0';
+ return rv;
+}
+
static uint64_t pmbus_receive_uint(PMBusDevice *pmdev)
{
diff --git a/hw/sensor/adm1266.c b/hw/sensor/adm1266.c
index 37d1cffd57..81545385a5 100644
--- a/hw/sensor/adm1266.c
+++ b/hw/sensor/adm1266.c
@@ -142,16 +142,15 @@ static int adm1266_write_data(PMBusDevice *pmdev, const uint8_t *buf,
switch (pmdev->code) {
case PMBUS_MFR_ID: /* R/W block */
- pmbus_receive_block(pmdev, (uint8_t *)s->mfr_id, sizeof(s->mfr_id));
+ pmbus_receive_string(pmdev, s->mfr_id, sizeof(s->mfr_id));
break;
case PMBUS_MFR_MODEL: /* R/W block */
- pmbus_receive_block(pmdev, (uint8_t *)s->mfr_model,
- sizeof(s->mfr_model));
+ pmbus_receive_string(pmdev, s->mfr_model, sizeof(s->mfr_model));
break;
case PMBUS_MFR_REVISION: /* R/W block*/
- pmbus_receive_block(pmdev, (uint8_t *)s->mfr_rev, sizeof(s->mfr_rev));
+ pmbus_receive_string(pmdev, s->mfr_rev, sizeof(s->mfr_rev));
break;
case ADM1266_SET_RTC: /* do nothing */
diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index f195c11384..4c4d3e5ae2 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -518,6 +518,16 @@ void pmbus_send_string(PMBusDevice *state, const char *data);
*/
uint8_t pmbus_receive_block(PMBusDevice *pmdev, uint8_t *dest, size_t len);
+/**
+ * @brief Receive a Block Write and store it as a NUL-terminated string.
+ * Write-side mirror of pmbus_send_string(): guarantees dest is always a valid
+ * C string so later reads cannot run past the field. Use for string registers
+ * (e.g. MFR_ID, MFR_MODEL); use pmbus_receive_block() for binary blocks.
+ * @param dest - string buffer with enough capacity to receive the write
+ * @param len - the capacity of dest (the last byte is reserved for the NUL)
+ */
+uint8_t pmbus_receive_string(PMBusDevice *pmdev, char *dest, size_t len);
+
/**
* @brief Receive data over PMBus
* These methods help track how much data is being received over PMBus
--
2.43.0On Sun, 14 Jun 2026 at 23:22, imaginos <imaginos32@gmail.com> wrote: > > The PMBus string registers (e.g. MFR_MODEL on the ADM1266) are writable > by the guest. pmbus_receive_block() can fill the destination field > completely, leaving no NUL terminator. When the value is later read back, > pmbus_send_string() calls strlen() on it, which reads past the end of the > array and returns a length that trips > > g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN); > > aborting QEMU. This is guest-triggerable. > > Add pmbus_receive_string(), the write-side mirror of pmbus_send_string(), > which reserves the last byte of the destination so the stored value is > always NUL-terminated, and use it for the ADM1266 MFR string registers. > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3388 > Signed-off-by: imaginos <imaginos32@gmail.com> Thanks for looking at this. Notably, the bug report in #3388 makes a completely incorrect claim about what its repro case is actually doing. The repro case does the "write 32 bytes to trash the NUL terminator, then try to read back", which is what this patch addresses. I'll let the pmbus maintainers review the patch from a technical point of view. -- PMM
Ping: Titus, could you review this patch, please? It would be good to get a fix into the upcoming release. I think on another thread you said also you had some patches to upstream to fix the "guest asks for data multiple times but never actually reads it" potential issue ? thanks -- PMM On Mon, 22 Jun 2026 at 11:18, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Sun, 14 Jun 2026 at 23:22, imaginos <imaginos32@gmail.com> wrote: > > > > The PMBus string registers (e.g. MFR_MODEL on the ADM1266) are writable > > by the guest. pmbus_receive_block() can fill the destination field > > completely, leaving no NUL terminator. When the value is later read back, > > pmbus_send_string() calls strlen() on it, which reads past the end of the > > array and returns a length that trips > > > > g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN); > > > > aborting QEMU. This is guest-triggerable. > > > > Add pmbus_receive_string(), the write-side mirror of pmbus_send_string(), > > which reserves the last byte of the destination so the stored value is > > always NUL-terminated, and use it for the ADM1266 MFR string registers. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3388 > > Signed-off-by: imaginos <imaginos32@gmail.com> > > Thanks for looking at this. Notably, the bug report in #3388 makes > a completely incorrect claim about what its repro case is actually > doing. The repro case does the "write 32 bytes to trash the NUL > terminator, then try to read back", which is what this patch addresses. > > I'll let the pmbus maintainers review the patch from a technical > point of view. > > -- PMM
The adm1266 has a strange access pattern for the MFR_* registers, I'm creating a series with the patch that fixes it, but a quick summary: the data written to the register is the length of the data to be read back, the actual string contents are read only. I'm sending the patches that update SMBus and the adm1266 up. -Titus
Thanks for catching that. I went through the repro myself and arrived at the same root cause, but I missed that the original report's description of it was inaccurate and should have flagged that. Thanks for the review, I appreciate you taking the time to look into it. пон, 22. јун 2026. у 12:18 Peter Maydell <peter.maydell@linaro.org> је написао/ла: > On Sun, 14 Jun 2026 at 23:22, imaginos <imaginos32@gmail.com> wrote: > > > > The PMBus string registers (e.g. MFR_MODEL on the ADM1266) are writable > > by the guest. pmbus_receive_block() can fill the destination field > > completely, leaving no NUL terminator. When the value is later read back, > > pmbus_send_string() calls strlen() on it, which reads past the end of the > > array and returns a length that trips > > > > g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN); > > > > aborting QEMU. This is guest-triggerable. > > > > Add pmbus_receive_string(), the write-side mirror of pmbus_send_string(), > > which reserves the last byte of the destination so the stored value is > > always NUL-terminated, and use it for the ADM1266 MFR string registers. > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3388 > > Signed-off-by: imaginos <imaginos32@gmail.com> > > Thanks for looking at this. Notably, the bug report in #3388 makes > a completely incorrect claim about what its repro case is actually > doing. The repro case does the "write 32 bytes to trash the NUL > terminator, then try to read back", which is what this patch addresses. > > I'll let the pmbus maintainers review the patch from a technical > point of view. > > -- PMM >
© 2016 - 2026 Red Hat, Inc.