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