> 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