From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44299 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OSCup-0002Hf-Ve for qemu-devel@nongnu.org; Fri, 25 Jun 2010 13:42:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OSCdN-0002wd-FA for qemu-devel@nongnu.org; Fri, 25 Jun 2010 13:24:10 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:57986) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OSCdN-0002vV-AK for qemu-devel@nongnu.org; Fri, 25 Jun 2010 13:24:09 -0400 Received: by vws10 with SMTP id 10so4908262vws.4 for ; Fri, 25 Jun 2010 10:24:04 -0700 (PDT) Message-ID: <4C24E5F5.2030501@gmail.com> Date: Fri, 25 Jun 2010 13:23:01 -0400 From: TJ MIME-Version: 1.0 Subject: Re: [Qemu-devel] Guest OS hangs on usb_add References: <4C22E300.5020109@gmail.com> <1277483535.5325.5.camel@qabil.uk.xensource.com> In-Reply-To: <1277483535.5325.5.camel@qabil.uk.xensource.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gianni Tedesco Cc: "qemu-devel@nongnu.org" On 06/25/10 12:32, Gianni Tedesco wrote: > A device MAY provide extended descriptors in 2 ways mentioned in the > spec, but ISTR finding at least one device in the wild with standard > descriptors extended which were not so much used by the "host" but by > application software. So not sure about your patch, a quirks blacklist > based on idDevice/idProduct might be the better fix here. Makes sense. I should add vend/prod id check. > However the more serious problem is spinning on zero length descriptor > when truncated descriptors are not valid and zero length (in fact < 2) > is totally unacceptable. Following patch checks for truncation. Gianni, Please check my later patch submitted last night. I basically did the same thing you did, but with few differences: - if descriptor size is < 2, goto fail - if the descriptor is USB_DT_CONFIG, we can skip through all the sub descriptors using wTotalLength field. - otherwise, simply skip it One thing to also watch out for is the string descriptors. I might be wrong, but it appears (from reading the doc) that string descriptors (at least for the device descriptor) can be interspersed with the config descriptors, in which case (config_descr_len < USB_DT_CONFIG_SIZE) without checking descriptor type might unwittingly lead to failure. -TJ > diff --git a/hw/usb.h b/hw/usb.h > index 00d2802..efd4a65 100644 > --- a/hw/usb.h > +++ b/hw/usb.h > @@ -117,6 +117,14 @@ > #define USB_DT_INTERFACE 0x04 > #define USB_DT_ENDPOINT 0x05 > > +/* > + * Descriptor sizes per descriptor type > + */ > +#define USB_DT_DEVICE_SIZE 18 > +#define USB_DT_CONFIG_SIZE 9 > +#define USB_DT_INTERFACE_SIZE 9 > +#define USB_DT_ENDPOINT_SIZE 7 > + > #define USB_ENDPOINT_XFER_CONTROL 0 > #define USB_ENDPOINT_XFER_ISOC 1 > #define USB_ENDPOINT_XFER_BULK 2 > diff --git a/usb-linux.c b/usb-linux.c > index 88273ff..d259290 100644 > --- a/usb-linux.c > +++ b/usb-linux.c > @@ -299,7 +299,7 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration) > > i = 0; > dev_descr_len = dev->descr[0]; > - if (dev_descr_len > dev->descr_len) { > + if ( dev_descr_len < USB_DT_DEVICE_SIZE || dev_descr_len > dev->descr_len) { > goto fail; > } > > @@ -314,6 +314,8 @@ static int usb_host_claim_interfaces(USBHostDevice *dev, int configuration) > continue; > } > config_descr_len = dev->descr[i]; > + if ( config_descr_len < USB_DT_CONFIG_SIZE ) > + goto fail; > > printf("husb: config #%d need %d\n", dev->descr[i + 5], configuration); >