[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] scsi: esp: check length before dma read
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] scsi: esp: check length before dma read |
Date: |
Wed, 15 Jun 2016 14:29:28 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 |
On 15/06/2016 14:11, Laszlo Ersek wrote:
> (1) In my opinion, this check is not sufficient. All of the following
> objects:
>
> - the "len" local variable
> - the "ESPState.dma_left" field
> - the "ESPState.cmdlen" field
>
> have type "uint32_t" (that is, "unsigned int"). Therefore the addition
> on the LHS is performed in "unsigned int", resulting in (well-defined,
> but still harmful) wrapping at 2^32.
True, on the other hand if s->do_cmd is 1 then dma_left is at most 32
(see handle_ti).
So a better fix is to change cmdbuf[] to 32 bytes in
include/hw/scsi/esp.h, and define a constant ESP_CMDBUF_SZ equal to 32
that can be used in handle_ti and in the definition of cmdbuf.
In addition, there is some useless code duplication between esp_do_dma
and handle_ti that can be fixed like this:
diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 3f08598..43d10f6 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -245,21 +245,17 @@ static void esp_do_dma(ESPState *s)
uint32_t len;
int to_device;
- to_device = (s->ti_size < 0);
len = s->dma_left;
if (s->do_cmd) {
trace_esp_do_dma(s->cmdlen, len);
s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
- s->ti_size = 0;
- s->cmdlen = 0;
- s->do_cmd = 0;
- do_cmd(s, s->cmdbuf);
return;
}
if (s->async_len == 0) {
/* Defer until data is available. */
return;
}
+ to_device = (s->ti_size < 0);
if (len > s->async_len) {
len = s->async_len;
}
@@ -358,13 +354,13 @@ static void handle_ti(ESPState *s)
s->dma_left = minlen;
s->rregs[ESP_RSTAT] &= ~STAT_TC;
esp_do_dma(s);
- } else if (s->do_cmd) {
+ }
+ if (s->do_cmd) {
trace_esp_handle_ti_cmd(s->cmdlen);
s->ti_size = 0;
s->cmdlen = 0;
s->do_cmd = 0;
do_cmd(s, s->cmdbuf);
- return;
}
}
> (2) I think the check may be off-by-one. If (s->cmdlen + len) equal
> sizeof(s->cmdbuf), that should be allowed, shouldn't it? Then the
> dma_memory_read() function just after will access the cmd buffer right
> to its end, but not after.
Also correct.
Paolo