Re: [PATCH 6/6] hw/i2c: Implement NPCM7XX SMBus Module FIFO Mode

[wuhaotsh--- via <qemu-devel@nongnu.org>]

[-- Attachment #1: Type: text/plain, Size: 1154 bytes --]

On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard <minyard@acm.org> wrote:

> On Tue, Jan 26, 2021 at 11:32:37AM -0800, wuhaotsh--- via wrote:
> > +
> > +static void npcm7xx_smbus_read_byte_fifo(NPCM7xxSMBusState *s)
> > +{
> > +    uint8_t received_bytes = NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts);
> > +
> > +    if (received_bytes == 0) {
> > +        npcm7xx_smbus_recv_fifo(s);
> > +        return;
> > +    }
> > +
> > +    s->sda = s->rx_fifo[s->rx_cur];
> > +    s->rx_cur = (s->rx_cur + 1u) % NPCM7XX_SMBUS_FIFO_SIZE;
> > +    --s->rxf_sts;
>
> This open-coded decrement seems a little risky.  Are you sure in every
> case that s->rxf_sts > 0?  There's no way what's running in the VM can
> game this and cause a buffer overrun?  One caller to this function seems
> to protect against this, and another does not.
>
s->rxf_sts is uint8_t so it's guaranteed to be >=0.
In the case s->rxf_sts == 0,  NPCM7XX_SMBRXF_STS_RX_BYTES(s->rxf_sts) is
also 0, so it'll take the if-branch and return without running --s->rxf_sts.
I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.

>
> Other than this, I didn't see any issues with this patch.
>
> -corey
>

[-- Attachment #2: Type: text/html, Size: 1874 bytes --]