* [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before read/write
@ 2016-05-31 11:36 P J P
2016-06-06 11:48 ` P J P
2016-06-06 12:00 ` Paolo Bonzini
0 siblings, 2 replies; 4+ messages in thread
From: P J P @ 2016-05-31 11:36 UTC (permalink / raw)
To: Qemu Developers
Cc: Peter Maydell, Paolo Bonzini, Huawei PSIRT, Li Qiang,
Prasad J Pandit
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.
Add check to avoid OOB r/w access.
Reported-by: Huawei PSIRT <PSIRT@huawei.com>
Reported-by: Li Qiang <liqiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/scsi/esp.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
Update as per
-> https://lists.gnu.org/archive/html/qemu-devel/2016-05/msg05506.html
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 591c817..60c1b28 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -400,16 +400,14 @@ uint64_t esp_reg_read(ESPState *s, uint32_t saddr)
trace_esp_mem_readb(saddr, s->rregs[saddr]);
switch (saddr) {
case ESP_FIFO:
- if (s->ti_size > 0) {
+ if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
+ /* Data out. */
+ qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
+ s->rregs[ESP_FIFO] = 0;
+ esp_raise_irq(s);
+ } else if (s->ti_rptr < s->ti_wptr) {
s->ti_size--;
- if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
- /* Data out. */
- qemu_log_mask(LOG_UNIMP,
- "esp: PIO data read not implemented\n");
- s->rregs[ESP_FIFO] = 0;
- } else {
- s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
- }
+ s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
esp_raise_irq(s);
}
if (s->ti_size == 0) {
@@ -456,7 +454,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++;
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before read/write
2016-05-31 11:36 [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before read/write P J P
@ 2016-06-06 11:48 ` P J P
2016-06-06 12:00 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: P J P @ 2016-06-06 11:48 UTC (permalink / raw)
To: Qemu Developers; +Cc: Li Qiang, Peter Maydell, Huawei PSIRT, Paolo Bonzini
+-- On Tue, 31 May 2016, P J P wrote --+
| switch (saddr) {
| case ESP_FIFO:
| - if (s->ti_size > 0) {
| + if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
| + /* Data out. */
| + qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
| + s->rregs[ESP_FIFO] = 0;
| + esp_raise_irq(s);
| + } else if (s->ti_rptr < s->ti_wptr) {
| s->ti_size--;
| - if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
| - /* Data out. */
| - qemu_log_mask(LOG_UNIMP,
| - "esp: PIO data read not implemented\n");
| - s->rregs[ESP_FIFO] = 0;
| - } else {
| - s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
| - }
| + s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
| esp_raise_irq(s);
| }
| if (s->ti_size == 0) {
| @@ -456,7 +454,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++;
Ping..!
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before read/write
2016-05-31 11:36 [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before read/write P J P
2016-06-06 11:48 ` P J P
@ 2016-06-06 12:00 ` Paolo Bonzini
2016-06-06 12:03 ` Peter Maydell
1 sibling, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2016-06-06 12:00 UTC (permalink / raw)
To: P J P, Qemu Developers
Cc: Li Qiang, Peter Maydell, Huawei PSIRT, Prasad J Pandit
On 31/05/2016 13:36, P J P wrote:
> + if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
> + /* Data out. */
> + qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
> + s->rregs[ESP_FIFO] = 0;
> + esp_raise_irq(s);
> + } else if (s->ti_rptr < s->ti_wptr) {
> s->ti_size--;
> - if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
> - /* Data out. */
> - qemu_log_mask(LOG_UNIMP,
> - "esp: PIO data read not implemented\n");
> - s->rregs[ESP_FIFO] = 0;
> - } else {
> - s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
> - }
> + s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
> esp_raise_irq(s);
> }
> if (s->ti_size == 0) {
Shouldn't this become a "s->ti_rptr == s->ti_wptr" too?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before read/write
2016-06-06 12:00 ` Paolo Bonzini
@ 2016-06-06 12:03 ` Peter Maydell
0 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2016-06-06 12:03 UTC (permalink / raw)
To: Paolo Bonzini
Cc: P J P, Qemu Developers, Li Qiang, Huawei PSIRT, Prasad J Pandit
On 6 June 2016 at 13:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 31/05/2016 13:36, P J P wrote:
>> + if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
>> + /* Data out. */
>> + qemu_log_mask(LOG_UNIMP, "esp: PIO data read not implemented\n");
>> + s->rregs[ESP_FIFO] = 0;
>> + esp_raise_irq(s);
>> + } else if (s->ti_rptr < s->ti_wptr) {
>> s->ti_size--;
>> - if ((s->rregs[ESP_RSTAT] & STAT_PIO_MASK) == 0) {
>> - /* Data out. */
>> - qemu_log_mask(LOG_UNIMP,
>> - "esp: PIO data read not implemented\n");
>> - s->rregs[ESP_FIFO] = 0;
>> - } else {
>> - s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
>> - }
>> + s->rregs[ESP_FIFO] = s->ti_buf[s->ti_rptr++];
>> esp_raise_irq(s);
>> }
>> if (s->ti_size == 0) {
>
> Shouldn't this become a "s->ti_rptr == s->ti_wptr" too?
It would probably be helpful to document what the intended invariant
relating ti_rptr, ti_wptr and ti_size is as well.
thanks
-- PMM
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-06-06 12:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-31 11:36 [Qemu-devel] [PATCH v2] scsi: esp: check TI buffer index before read/write P J P
2016-06-06 11:48 ` P J P
2016-06-06 12:00 ` Paolo Bonzini
2016-06-06 12:03 ` Peter Maydell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).