NetBSD Problem Report #43475

From www@NetBSD.org  Mon Jun 14 22:24:53 2010
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 6DFB463B8DF
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 14 Jun 2010 22:24:53 +0000 (UTC)
Message-Id: <20100614222453.30F5B63B8CE@www.NetBSD.org>
Date: Mon, 14 Jun 2010 22:24:53 +0000 (UTC)
From: jasper@pointless.net
Reply-To: jasper@pointless.net
To: gnats-bugs@NetBSD.org
Subject: MTK 747 A+ GPS recorder has a bogus descriptor
X-Send-Pr-Version: www-1.0

>Number:         43475
>Category:       kern
>Synopsis:       MTK 747 A+ GPS recorder has a bogus descriptor
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jun 14 22:25:00 +0000 2010
>Last-Modified:  Tue Jun 15 23:05:04 +0000 2010
>Originator:     Jasper Wallace
>Release:        -current
>Organization:
>Environment:
NetBSD limpit 5.99.30 NetBSD 5.99.30 (GENERIC) #0: Wed Jun  9 20:51:58 BST 2010  jasper@limpit:/home/jasper/develop/netbsd/netbsd-src-and-build/tree/n.64/obj/sys/arch/amd64/compile/GENERIC amd64
>Description:
The MTK 747 A+ GPS recorder is a CDM/ACM usb device, but the length of the 1st interface descriptor is the sum of the length of all the interface descriptors, so any interface descriptor parser assumes there is only one interface descriptor and ignores all the CDC/ACM descriptors which means the driver fails to attach.


>How-To-Repeat:
connect a MTK A+ Gps recorder to a netbsd system, watch it fail to attach.
>Fix:
This patch also adds a check to usbdi usb_desc_iter functions to see if len(first item) is > sizeof(usb_interface_descriptor_t) which should make it eaiser to catch this kind of thing if it happens again. It does mean that is any driver tries to use the iter functions from part way through a descriptor they are going to get random warning messages from the kernel. (i.e. the change to usbdi may not be a brilliant idea), also it should probably be using something other than printf there, maybe wrap the whole usbdi thing in #ifdef DEBUG?

patch (hope it dosn't get mangled by the webinterface):

Index: umodem_common.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/umodem_common.c,v
retrieving revision 1.17
diff -u -u -r1.17 umodem_common.c
--- umodem_common.c	12 Nov 2009 19:58:27 -0000	1.17
+++ umodem_common.c	14 Jun 2010 22:21:42 -0000
@@ -369,6 +369,7 @@
 	const usb_cdc_cm_descriptor_t *cmd;
 	const usb_cdc_acm_descriptor_t *cad;
 	const usb_cdc_union_descriptor_t *cud;
+	int ret;

 	*cm = *acm = 0;

@@ -399,6 +400,55 @@
 		DPRINTF(("umodem_get_caps: no UNION desc\n"));
 	}

+	ret = cmd ? cmd->bDataInterface : cud ? cud->bSlaveInterface[0] : -1;
+	if (ret != -1)
+		return ret;
+
+	DPRINTF(("%s: probe failed, main id length: %u\n", __FUNCTION__, id->bLength));
+
+	/* is the interface descriptor mis sized? */
+	if (id->bLength > sizeof(usb_interface_descriptor_t))
+	{
+		int pos;
+		const usb_cdc_descriptor_t *d;
+		const usb_cdc_header_descriptor_t *header;
+		const unsigned char *desc_ptr;
+
+		pos = sizeof(usb_interface_descriptor_t);
+		desc_ptr = (const unsigned char *)id;
+		
+		while (pos < id->bLength)
+		{
+			d = (const void *)(desc_ptr + pos);
+			DPRINTF(("len: %x type: %02x subtype: %x\n", d->bLength,
+						d->bDescriptorType,
+						d->bDescriptorSubtype));
+			pos += d->bLength;
+			if (d->bDescriptorType == UDESC_CS_INTERFACE)
+			{
+				/* if it's not an interface type we should probably stop probing */
+				switch (d->bDescriptorSubtype) {
+				case UDESCSUB_CDC_HEADER:
+					header = (const usb_cdc_header_descriptor_t *)d;
+					break;
+				case UDESCSUB_CDC_CM:
+					cmd = (const usb_cdc_cm_descriptor_t *)d;
+					*cm = cmd->bmCapabilities;
+					break;
+				case UDESCSUB_CDC_ACM:
+					cad = (const usb_cdc_acm_descriptor_t *)d;
+					*acm = cad->bmCapabilities;
+					break;
+				case UDESCSUB_CDC_UNION:
+					cud = (const usb_cdc_union_descriptor_t *)d;
+					break;
+				default:
+					DPRINTF(("CDC device, but descriptor subtype %d not known\n", d->bDescriptorSubtype));
+				}
+			}
+		}
+		
+	}
 	return cmd ? cmd->bDataInterface : cud ? cud->bSlaveInterface[0] : -1;
 }

Index: usb_quirks.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/usb_quirks.c,v
retrieving revision 1.66
diff -u -u -r1.66 usb_quirks.c
--- usb_quirks.c	14 Mar 2010 08:44:46 -0000	1.66
+++ usb_quirks.c	14 Jun 2010 22:21:42 -0000
@@ -94,6 +94,8 @@
         0x100, { UQ_ASSUME_CM_OVER_DATA }},
  { USB_VENDOR_SIEMENS2, USB_PRODUCT_SIEMENS2_MC75,
         0x000, { UQ_ASSUME_CM_OVER_DATA }},
+ { USB_VENDOR_MTK, USB_PRODUCT_MTK_APLUSGPS,
+        ANY, { UQ_ASSUME_CM_OVER_DATA }},        
  { USB_VENDOR_TELEX, USB_PRODUCT_TELEX_MIC1,	    0x009, { UQ_AU_NO_FRAC }},
  { USB_VENDOR_SILICONPORTALS, USB_PRODUCT_SILICONPORTALS_YAPPHONE,
    						    0x100, { UQ_AU_INP_ASYNC }},
Index: usbdevs
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/usbdevs,v
retrieving revision 1.552
diff -u -u -r1.552 usbdevs
--- usbdevs	6 Jun 2010 23:01:05 -0000	1.552
+++ usbdevs	14 Jun 2010 22:21:44 -0000
@@ -413,6 +413,7 @@
 vendor SITECOMEU	0x0df6	Sitecom Europe
 vendor HAWKING		0x0e66	Hawking
 vendor GMATE		0x0e7e	G.Mate, Inc
+vendor MTK 		0x0e8d	MTK
 vendor OTI		0x0ea0	Ours Technology
 vendor PILOTECH		0x0eaf	Pilotech
 vendor NOVATECH		0x0eb0	Nova Tech
@@ -1697,6 +1698,9 @@
 product MUSTEK MDC800		0xa800	MDC-800 digital camera
 product MUSTEK DV2000		0xc441	DV2000 digital camera

+/* MTK products */
+product MTK APLUSGPS 		0x3329 A+ GPS Recorder
+
 /* National Instruments */
 product NI GPIB_USB_A		0xc920	GPIB-USB-A

Index: usbdi.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/usbdi.c,v
retrieving revision 1.127
diff -u -u -r1.127 usbdi.c
--- usbdi.c	16 Jan 2010 17:03:03 -0000	1.127
+++ usbdi.c	14 Jun 2010 22:21:48 -0000
@@ -1129,6 +1129,7 @@

         iter->cur = (const uByte *)cd;
         iter->end = (const uByte *)cd + UGETW(cd->wTotalLength);
+        iter->start = 1;
 }

 const usb_descriptor_t *
@@ -1146,6 +1147,14 @@
 		printf("usb_desc_iter_next: descriptor length = 0\n");
 		return NULL;
 	}
+
+	/* if the 1st descriptor is longer than 9 it could be bogus */
+	if (iter->start) {
+	        if (desc->bLength > sizeof(usb_interface_descriptor_t)) {
+	                printf("%s: first descriptor has wrong length?: %d\n", __FUNCTION__, desc->bLength);
+                }
+                iter->start = 0;
+        }
 	iter->cur += desc->bLength;
 	if (iter->cur > iter->end) {
 		printf("usb_desc_iter_next: descriptor length too large\n");
Index: usbdi.h
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/usbdi.h,v
retrieving revision 1.79
diff -u -u -r1.79 usbdi.h
--- usbdi.h	4 Sep 2009 17:53:12 -0000	1.79
+++ usbdi.h	14 Jun 2010 22:21:48 -0000
@@ -185,6 +185,7 @@
 typedef struct {
 	const uByte *cur;
 	const uByte *end;
+	bool start;
 } usbd_desc_iter_t;
 void usb_desc_iter_init(usbd_device_handle, usbd_desc_iter_t *);
 const usb_descriptor_t *usb_desc_iter_next(usbd_desc_iter_t *);

>Audit-Trail:
From: "Jonathan A. Kollasch" <jakllsch@kollasch.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43475: MTK 747 A+ GPS recorder has a bogus descriptor
Date: Tue, 15 Jun 2010 21:09:45 +0000

 So, you (were) get(ing) a "umodem0: no pointer to data interface" error?

 Taking a hint from Linux and cdce(4), it would appear something
 along the lines of the CDCE_NO_UNION quirk would be more appropriate.

 Also, I'd sooner see USB_PRODUCT_MEDIATEK_MT3329, as this ID appears
 to be used in multiple products based on the MT3329 chip, and "MTK"
 doesn't seem similar enough to the name listed in the USB-IF vendor
 number database.

From: Jasper Wallace <jasper@pointless.net>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: kern/43475: MTK 747 A+ GPS recorder has a bogus descriptor
Date: Tue, 15 Jun 2010 22:33:50 +0100 (BST)

 On Tue, 15 Jun 2010, Jonathan A. Kollasch wrote:

 > The following reply was made to PR kern/43475; it has been noted by GNATS.
 > 
 > From: "Jonathan A. Kollasch" <jakllsch@kollasch.net>
 > To: gnats-bugs@NetBSD.org
 > Cc: 
 > Subject: Re: kern/43475: MTK 747 A+ GPS recorder has a bogus descriptor
 > Date: Tue, 15 Jun 2010 21:09:45 +0000
 > 
 >  So, you (were) get(ing) a "umodem0: no pointer to data interface" error?

 I can't remember what the error was, but there definatly wasn't a ucom 
 attachment, i can try with an unpatched kernel if it's important.

 >  Taking a hint from Linux and cdce(4), it would appear something
 >  along the lines of the CDCE_NO_UNION quirk would be more appropriate.

 Maybe, but this device does have a cdc union, it's just hidden behind the 
 bogus descriptor length. Also fixing this without a quirk may make the fix 
 just work with a larger range of devices and save someone having to work 
 out what the problem is and patch the kernel to make the device work for 
 them.

 >  Also, I'd sooner see USB_PRODUCT_MEDIATEK_MT3329, as this ID appears
 >  to be used in multiple products based on the MT3329 chip, and "MTK"
 >  doesn't seem similar enough to the name listed in the USB-IF vendor
 >  number database.

 Whatever is appropiate, it's not particulary important.

 -- 
 [http://pointless.net/]                                   [0x2ECA0975]

Home	PR Database Search