Re: [PATCH] hw/i2c/pmbus_device: Fix a possible crash when requesting too many bytes

On Tue, 9 Jun 2026 at 04:01, Peter Maydell <[email protected]> wrote:
>
> On Tue, 9 Jun 2026 at 11:56, Thomas Huth <[email protected]> wrote:
> >
> > From: Thomas Huth <[email protected]>
> >
> > The pmbus_send_string() function contains an assert() statement that
> > can be triggered by the guest code when requesting too many data
> > without reading from the device in between. This should not be possible.
> > pmbus_send() already has a similar logic, but it simply ignores the
> > error after logging a message with qemu_log_mask(), so do the same now
> > in pmbus_send_string() to fix the issue.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3388
> > Signed-off-by: Thomas Huth <[email protected]>
> > ---
> >  hw/i2c/pmbus_device.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> > index b1f9843f52e..6aa608a2998 100644
> > --- a/hw/i2c/pmbus_device.c
> > +++ b/hw/i2c/pmbus_device.c
> > @@ -104,7 +104,12 @@ void pmbus_send_string(PMBusDevice *pmdev, const char *data)
> >      }
> >
> >      size_t len = strlen(data);
> > -    g_assert(len + pmdev->out_buf_len < SMBUS_DATA_MAX_LEN);
> > +    if (len + pmdev->out_buf_len >= SMBUS_DATA_MAX_LEN) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: requested too much data from PMBus device\n",
> > +                      __func__);
> > +        return;
> > +    }
>
> I had a look at this bug earlier, but was not sure that this is the
> right thing. What happens is that the guest does something that
> causes the device to queue up data X ready for the guest to
> read, but then instead of reading it, the guest does another
> "I would like data X please" action. My guess is that the way
> the hardware handles this is probably not "add the second
> copy of data X after the first one". Perhaps it is "drop the
> data the guest didn't read, so the next guest read gets the
> second lot of data, not the first". But maybe the spec really
> does say "you can do things in the order 'ask for A, ask for
> B, read data for A, read data for B".
>
> I couldn't conveniently find the pmbus spec to find out what
> the hardware is supposed to do here. We need some input from
> somebody who knows about pmbus.
>
> thanks
> -- PMM

Queueing reads are an implementation detail I decided on, not part of
the spec. How the data gets discarded is up to the device
manufacturer.
There's a separate issue here though that I have a fix for.
SMBUS_DATA_MAX_LEN in QEMU is for an older SMBus version and it needs
to be increased.

I can set aside some time to bring the SMBus implementation up to
date, but for now increasing it to 255 will put QEMU in line SMBus
v3.3.
-Titus