Re: [Qemu-devel] [PATCH] scsi: esp: check TI buffer index before read/write

[P J P <ppandit@redhat.com>]

  Hello Peter,

+-- On Mon, 30 May 2016, Peter Maydell wrote --+
| > +            } 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?

  OOB read occurs when 's->ti_rptr' exceeds 'TI_BUFSZ'.
 
| 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.
|
| > -        } else if (s->ti_size == TI_BUFSZ - 1) {
| > +        } else if (s->ti_wptr == TI_BUFSZ - 1) {
| >              trace_esp_error_fifo_overrun();
|
| 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.

  Both issues occur as guest could control the value of 's->ti_size' by 
alternating between 'esp_reg_write(, ESP_FIFO, )' & 'esp_reg_read(, ESP_FIFO)' 
calls. One increases 's->ti_size' and other descreases the same.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F