scsi: esp: check TI buffer index before read/write

From: Prasad J Pandit <pjp@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 <PSIRT@huawei.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/scsi/esp.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

On 30 May 2016 at 19:58, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@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 <PSIRT@huawei.com>
> Signed-off-by: Prasad J Pandit <pjp@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

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

On 31 May 2016 at 06:20, P J P <ppandit@redhat.com> wrote:
>   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'.

So? This is a FIFO *read*, and the trace message is for
the case when the FIFO is too full, which can't happen
for a read.

> | 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.

OK, so we need to fix these code paths so that they keep
all of the read, write and size values in sync (or just
rewrite the code so it isn't tracking three values, since
two out of three should determine the other.

thanks
-- PMM

+-- On Tue, 31 May 2016, Peter Maydell wrote --+
| This is a FIFO *read*, and the trace message is for the case when the FIFO 
| is too full, which can't happen for a read.

  Okay.

| OK, so we need to fix these code paths so that they keep all of the read, 
| write and size values in sync (or just rewrite the code so it isn't tracking 
| three values, since two out of three should determine the other.

  I've sent a revised patch v2.

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