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

[Corey Minyard <minyard@acm.org>]

On Wed, Jan 27, 2021 at 12:37:46PM -0800, wuhaotsh--- via wrote:
> 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.

That is true if called from the
NPCM7XX_SMBUS_STATUS_STOPPING_LAST_RECEIVE case.  There is no such check
in the NPCM7XX_SMBUS_STATUS_RECEIVING case.

> I'll probably add "g_assert(s->rxf_sts > 0)" to clarify.

You never want to do an assert if the hosted system can do something to
cause it.  If you add the check to the NPCM7XX_SMBUS_STATUS_RECEIVING
case, it would be ok, but really unnecessary.

If it's fine if s->rxf_sts wraps to 0xff, then this all doesn't matter,
but you want to add a comment to that effect if so.  These sorts of
things look dangerous.

There is also the question about who takes these patches in.  I'm the
I2C maintainer, but there's other code in this series.  Once everything
is ready, I can ack them if we take it through the ARM tree.  Or I can
take it through my tree with the proper acks.

-corey

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