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]
(Contact us)
$NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.