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

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

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

While doing DMA read into ESP command buffer 's->cmdbuf', the
length parameter could exceed the buffer size. Add check to avoid
OOB access.

Reported-by: Li Qiang <qiang6-s@360.cn>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/scsi/esp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index 4b94bbc..dfea571 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -249,6 +249,9 @@ static void esp_do_dma(ESPState *s)
     len = s->dma_left;
     if (s->do_cmd) {
         trace_esp_do_dma(s->cmdlen, len);
+        if (s->cmdlen + len >= sizeof(s->cmdbuf)) {
+            return;
+        }
         s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
         s->ti_size = 0;
         s->cmdlen = 0;
-- 
2.5.5

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

[Laszlo Ersek]

On 06/15/16 11:29, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> While doing DMA read into ESP command buffer 's->cmdbuf', the
> length parameter could exceed the buffer size. Add check to avoid
> OOB access.
> 
> Reported-by: Li Qiang <qiang6-s@360.cn>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/scsi/esp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
> index 4b94bbc..dfea571 100644
> --- a/hw/scsi/esp.c
> +++ b/hw/scsi/esp.c
> @@ -249,6 +249,9 @@ static void esp_do_dma(ESPState *s)
>      len = s->dma_left;
>      if (s->do_cmd) {
>          trace_esp_do_dma(s->cmdlen, len);
> +        if (s->cmdlen + len >= sizeof(s->cmdbuf)) {
> +            return;
> +        }
>          s->dma_memory_read(s->dma_opaque, &s->cmdbuf[s->cmdlen], len);
>          s->ti_size = 0;
>          s->cmdlen = 0;
> 

(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.

(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.


So, I suggest the following instead:

        if (s->cmdlen > sizeof(s->cmdbuf) ||
            len > sizeof(s->cmdbuf) - s->cmdlen) {
            return;
        }

The first subcondition may be constant false at this point, due to an
earlier check somewhere else in the code; I'm not sure. If that's the
case, then the first subcondition can be dropped.

Either way, the first subcondition makes sure that the subtraction in
the second subcondition will never underflow.

And the second subcondition expresses (without a possibility for
overflow) the actual check we're interested in.

Thanks
Laszlo

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

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

[P J P]

+-- On Wed, 15 Jun 2016, Paolo Bonzini wrote --+
| 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.

Sent a revised patch v3. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F