Re: [Qemu-devel] [PATCH] usb: check RNDIS buffer offsets & length

[Gerd Hoffmann]

> diff --git a/hw/usb/core.c b/hw/usb/core.c
> index d0025db..9d90ec7 100644
> --- a/hw/usb/core.c
> +++ b/hw/usb/core.c
> @@ -128,9 +128,16 @@ static void do_token_setup(USBDevice *s, USBPacket *p)
>      }
>  
>      usb_packet_copy(p, s->setup_buf, p->iov.size);
> +    s->setup_index = 0;
>      p->actual_length = 0;
>      s->setup_len   = (s->setup_buf[7] << 8) | s->setup_buf[6];
> -    s->setup_index = 0;
> +    if (s->setup_len > sizeof(s->data_buf)) {
> +        fprintf(stderr,
> +                "usb_generic_handle_packet: ctrl buffer too small (%d > 
> %zu)\n",
> +                s->setup_len, sizeof(s->data_buf));
> +        p->status = USB_RET_STALL;
> +        return;
> +    }
>  
>      request = (s->setup_buf[0] << 8) | s->setup_buf[1];
>      value   = (s->setup_buf[3] << 8) | s->setup_buf[2];
> @@ -151,13 +158,6 @@ static void do_token_setup(USBDevice *s, USBPacket *p)
>          }
>          s->setup_state = SETUP_STATE_DATA;
>      } else {
> -        if (s->setup_len > sizeof(s->data_buf)) {
> -            fprintf(stderr,
> -                "usb_generic_handle_packet: ctrl buffer too small (%d > 
> %zu)\n",
> -                s->setup_len, sizeof(s->data_buf));
> -            p->status = USB_RET_STALL;
> -            return;
> -        }
>          if (s->setup_len == 0)
>              s->setup_state = SETUP_STATE_ACK;
>          else

Moves up the check so it is done for every control xfer.  Good.

> @@ -172,11 +172,18 @@ static void do_token_in(USBDevice *s, USBPacket *p)
>      int request, value, index;
>  
>      assert(p->ep->nr == 0);
> +    if (s->setup_len > sizeof(s->data_buf)) {
> +        fprintf(stderr,
> +                "usb_generic_handle_packet: ctrl buffer too small (%d > 
> %zu)\n",
> +                s->setup_len, sizeof(s->data_buf));
> +        p->status = USB_RET_STALL;
> +        return;
> +    }

Why this is needed?  All control transfers go through do_token_setup
first, so with the check moved in do_token_setup we should never ever
trigger it here ...

> -    if (bufoffs + buflen > length)
> +    if (buflen > length || bufoffs >= length || bufoffs + buflen > length) {
>          return USB_RET_STALL;
> +    }

What is this?  Not mentioned in the commit message.  Looks like integer
overflow prevention to me (if correct: separate patch with proper commit
message please).

thanks,
  Gerd