On Tue, Jan 26, 2021 at 3:47 PM Corey Minyard <miny...@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 >