On 30 May 2016 at 19:58, P J P <ppan...@redhat.com> wrote: > From: Prasad J Pandit <p...@fedoraproject.org> > > The 53C9X Fast SCSI Controller(FSC) comes with internal 16-byte > FIFO buffers. One is used to handle commands and other is for > information transfer. While reading/writing to these buffers > an index into 's->ti_buf[TI_BUFSZ=16]' could exceed its size, > as a check was missing to validate it. Add check to avoid OOB > r/w access. > > Reported-by: Huawei PSIRT <ps...@huawei.com> > Signed-off-by: Prasad J Pandit <p...@fedoraproject.org> > --- > hw/scsi/esp.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c > index 591c817..80e4287 100644 > --- a/hw/scsi/esp.c > +++ b/hw/scsi/esp.c > @@ -407,8 +407,10 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr) > qemu_log_mask(LOG_UNIMP, > "esp: PIO data read not implemented\n"); > s->rregs[ESP_FIFO] = 0; > - } else { > + } else if (s->ti_rptr < TI_BUFSZ) { > s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++]; > + } else { > + trace_esp_error_fifo_overrun();
Isn't this an underrun, not an overrun? In any case, something weird seems to be going on here: it looks like the amount of data in the fifo should be constrained by ti_size (which we're already checking), so when can we get into a situation where ti_rptr can get beyond the buffer size? It seems to me that we should fix whatever that underlying bug is, and then have an assert() on ti_rptr here. > } > esp_raise_irq(s); > } > @@ -456,7 +458,7 @@ void esp_reg_write(ESPState *s, uint32_t saddr, uint64_t > val) > } else { > trace_esp_error_fifo_overrun(); > } > - } else if (s->ti_size == TI_BUFSZ - 1) { > + } else if (s->ti_wptr == TI_BUFSZ - 1) { > trace_esp_error_fifo_overrun(); > } else { > s->ti_size++; Similarly, this looks odd -- the ti_size check should be sufficient if the rest of the code is correctly managing the ti_size/ti_wptr/ti_rptr values. It would probably also be possible to rewrite this FIFO handling into a less error prone style that isn't reliant on keeping three different indexes in sync to avoid buffer overruns. > -- > 2.5.5 thanks -- PMM