[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] scsi: esp: check TI buffer index before read/wr
From: |
P J P |
Subject: |
Re: [Qemu-devel] [PATCH] scsi: esp: check TI buffer index before read/write |
Date: |
Tue, 31 May 2016 10:50:57 +0530 (IST) |
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