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