Re: [Qemu-devel] [PATCH] scsi: esp: check length before dma read

[Paolo Bonzini]

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