[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] usb: check RNDIS buffer offsets & length
From: |
Gerd Hoffmann |
Subject: |
Re: [Qemu-devel] [PATCH] usb: check RNDIS buffer offsets & length |
Date: |
Tue, 16 Feb 2016 15:33:50 +0100 |
> 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