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 <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  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