[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Laszlo Ersek
Subject: Re: [Qemu-devel] [PATCH] scsi: esp: check length before dma read
Date: Wed, 15 Jun 2016 14:11:22 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

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

- 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) {

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]