[PATCH] hw/i2c: pmbus: NUL-terminate strings received via block write

[PATCH] hw/i2c: pmbus: NUL-terminate strings received via block write

[imaginos]

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.0