From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36010) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVghP-00079T-PL for qemu-devel@nongnu.org; Tue, 16 Feb 2016 09:33:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVghM-0008Dh-HQ for qemu-devel@nongnu.org; Tue, 16 Feb 2016 09:33:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36022) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVghM-0008Dc-CS for qemu-devel@nongnu.org; Tue, 16 Feb 2016 09:33:52 -0500 Message-ID: <1455633230.7504.107.camel@redhat.com> From: Gerd Hoffmann Date: Tue, 16 Feb 2016 15:33:50 +0100 In-Reply-To: <1454669651-29411-1-git-send-email-ppandit@redhat.com> References: <1454669651-29411-1-git-send-email-ppandit@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Mime-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] usb: check RNDIS buffer offsets & length List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Qinghao Tang , Qemu Developers , Prasad J Pandit > 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) > } > =20 > usb_packet_copy(p, s->setup_buf, p->iov.size); > + s->setup_index =3D 0; > p->actual_length =3D 0; > s->setup_len =3D (s->setup_buf[7] << 8) | s->setup_buf[6]; > - s->setup_index =3D 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 =3D USB_RET_STALL; > + return; > + } > =20 > request =3D (s->setup_buf[0] << 8) | s->setup_buf[1]; > value =3D (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 =3D 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 =3D USB_RET_STALL; > - return; > - } > if (s->setup_len =3D=3D 0) > s->setup_state =3D 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; > =20 > assert(p->ep->nr =3D=3D 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 =3D 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 >=3D length || bufoffs + buflen > len= gth) { > 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