[PATCH] Devices that ignore USB spec generate invalid modaliases

[PATCH] Devices that ignore USB spec generate invalid modaliases

[Nathaniel McCallum]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

Please CC me as I'm not subscribed to LKML.

The current code to generate usb modaliases from usb_device_id assumes 
that the device's bcdDevice descriptor will actually be in BCD format. 
While this should be a sane assumption, some devices don't follow spec 
and just use plain old hex.  This causes drivers for these devices to 
generate invalid modalias lines which will never actually match for the 
hardware.

The following patch adds hex support for bcdDevice in file2alias.c. 
Drivers for devices which have bcdDevice conforming to BCD will have no 
change in modalias output.  Drivers for devices which don't conform 
(primarily usb-storage and ibmcam in my initial survey) should now 
generate valid modaliases.

EXAMPLE OUTPUT (ibmcam; space added to highlight change)
Old: usb:v0545p800D d030[10-9] dc*dsc*dp*ic*isc*ip*
New: usb:v0545p800D d030a      dc*dsc*dp*ic*isc*ip*

Patch attached. Questions/comments welcome.

Nathaniel McCallum

[-- Attachment #2: file2alias_usb_bcd2hex.patch --]
[-- Type: text/plain, Size: 1502 bytes --]

--- linux-2.6.31.orig/scripts/mod/file2alias.c	2009-09-09 18:13:59.000000000 -0400
+++ linux-2.6.31/scripts/mod/file2alias.c	2009-11-11 02:51:02.446894908 -0500
@@ -118,9 +118,20 @@
 		sprintf(alias + strlen(alias), "%0*X",
 			bcdDevice_initial_digits, bcdDevice_initial);
 	if (range_lo == range_hi)
-		sprintf(alias + strlen(alias), "%u", range_lo);
-	else if (range_lo > 0 || range_hi < 9)
-		sprintf(alias + strlen(alias), "[%u-%u]", range_lo, range_hi);
+		sprintf(alias + strlen(alias), "%x", range_lo);
+	else if (range_lo > 0x0 || range_hi < 0xf) {
+		if (range_lo > 0x9 || range_hi < 0xa)
+			sprintf(alias + strlen(alias),
+				"[%x-%x]", range_lo, range_hi);
+		else {
+			sprintf(alias + strlen(alias), 
+				range_lo < 0x9 ? "[%x-9" : "[%x",
+				range_lo);
+			sprintf(alias + strlen(alias), 
+				range_hi > 0xa ? "a-%x]" : "%x]",
+				range_hi);
+		}
+	}
 	if (bcdDevice_initial_digits < (sizeof(id->bcdDevice_lo) * 2 - 1))
 		strcat(alias, "*");
 
@@ -173,8 +184,6 @@
 	for (ndigits = sizeof(id->bcdDevice_lo) * 2 - 1; devlo <= devhi; ndigits--) {
 		clo = devlo & 0xf;
 		chi = devhi & 0xf;
-		if (chi > 9)	/* it's bcd not hex */
-			chi = 9;
 		devlo >>= 4;
 		devhi >>= 4;
 
@@ -184,10 +193,10 @@
 		}
 
 		if (clo > 0)
-			do_usb_entry(id, devlo++, ndigits, clo, 9, mod);
+			do_usb_entry(id, devlo++, ndigits, clo, 0xf, mod);
 
-		if (chi < 9)
-			do_usb_entry(id, devhi--, ndigits, 0, chi, mod);
+		if (chi < 0xf)
+			do_usb_entry(id, devhi--, ndigits, 0x0, chi, mod);
 	}
 }
 

Re: [PATCH] Devices that ignore USB spec generate invalid modaliases

[Nathaniel McCallum]

On 11/11/2009 03:20 AM, Nathaniel McCallum wrote:
> Drivers for devices which have bcdDevice conforming to BCD will have no
> change in modalias output.

One small clarification. Existing drivers which specify a non-zero lower 
bound and an upper bound past the next character will have their upper 
bound changed from 0x9 to 0xf (i.e.: d123[4-9] -> d123[4-9a-f]).  I 
believe this is the only case where an existing (working) modalias will 
change.

Nathaniel McCallum

Re: [PATCH][RFC] Devices that ignore USB spec generate invalid modaliases

[Nathaniel McCallum]

[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]

On 11/11/2009 03:39 AM, Nathaniel McCallum wrote:
> On 11/11/2009 03:20 AM, Nathaniel McCallum wrote:
>> Drivers for devices which have bcdDevice conforming to BCD will have no
>> change in modalias output.
>
> One small clarification. Existing drivers which specify a non-zero lower
> bound and an upper bound past the next character will have their upper
> bound changed from 0x9 to 0xf (i.e.: d123[4-9] -> d123[4-9a-f]). I
> believe this is the only case where an existing (working) modalias will
> change.

I've solved the above problem in the latest patch 
(file2alias_hex_or_bcd.patch) which automatically detects if a driver 
entry is supporting a hex bcdDevice rather than a bcd one and adjusts 
accordingly.  Thus, for all normal devices, there is now no change at all.

In the process I discovered another bug.  'devlo++' and 'devhi--' are 
used.  However this number is most often in BCD format, so ++/-- won't 
work.  A second patch (file2alias_bcd_increment.patch) resolves this issue.

Again, questions/comments welcome.

Nathaniel McCallum

[-- Attachment #2: file2alias_bcd_increment.patch --]
[-- Type: text/plain, Size: 1800 bytes --]

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 85ad782..f25c09b 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -160,6 +160,45 @@ static void do_usb_entry(struct usb_device_id *id,
 		   "MODULE_ALIAS(\"%s\");\n", alias);
 }
 
+/* Handles increment/decrement of BCD formatted integers */
+/* Returns the previous value, so it works like i++ or i-- */
+static unsigned int incbcd(unsigned int *bcd,
+			   int inc,
+			   unsigned char max,
+			   size_t chars)
+{
+	unsigned int init = *bcd, i, j;
+	unsigned long long c, dec = 0;
+
+	/* If bcd is not in BCD format, just increment */
+	if (max > 0x9) {
+		*bcd += inc;
+		return init;
+	}
+
+	/* Convert BCD to Decimal */
+	for (i=0 ; i < chars ; i++) {
+		c = (*bcd >> (i << 2)) & 0xf;
+		c = c > 9 ? 9 : c; /* force to bcd just in case */
+		for (j=0 ; j < i ; j++)
+			c = c * 10;
+		dec += c;
+	}
+
+	/* Do our increment/decrement */
+	dec += inc;
+	*bcd  = 0;
+
+	/* Convert back to BCD */
+	for (i=0 ; i < chars ; i++) {
+		for (c=1,j=0 ; j < i ; j++)
+			c = c * 10;
+		c = (dec / c) % 10;
+		*bcd += c << (i << 2);
+	}
+	return init;
+}
+
 static void do_usb_entry_multi(struct usb_device_id *id, struct module *mod)
 {
 	unsigned int devlo, devhi;
@@ -208,10 +247,16 @@ static void do_usb_entry_multi(struct usb_device_id *id, struct module *mod)
 		}
 
 		if (clo > 0x0)
-			do_usb_entry(id, devlo++, ndigits, clo, max, max, mod);
+			do_usb_entry(id,
+				     incbcd(&devlo, 1, max,
+					    sizeof(id->bcdDevice_lo) * 2),
+				     ndigits, clo, max, max, mod);
 
 		if (chi < max)
-			do_usb_entry(id, devhi--, ndigits, 0x0, chi, max, mod);
+			do_usb_entry(id,
+				     incbcd(&devhi, -1, max,
+					    sizeof(id->bcdDevice_lo) * 2),
+				     ndigits, 0x0, chi, max, mod);
 	}
 }
 

[-- Attachment #3: file2alias_hex_or_bcd.patch --]
[-- Type: text/plain, Size: 3250 bytes --]

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 62a9025..85ad782 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -104,7 +104,7 @@ static void device_id_check(const char *modname, const char *device_id,
 static void do_usb_entry(struct usb_device_id *id,
 			 unsigned int bcdDevice_initial, int bcdDevice_initial_digits,
 			 unsigned char range_lo, unsigned char range_hi,
-			 struct module *mod)
+			 unsigned char max, struct module *mod)
 {
 	char alias[500];
 	strcpy(alias, "usb:");
@@ -118,9 +118,22 @@ static void do_usb_entry(struct usb_device_id *id,
 		sprintf(alias + strlen(alias), "%0*X",
 			bcdDevice_initial_digits, bcdDevice_initial);
 	if (range_lo == range_hi)
-		sprintf(alias + strlen(alias), "%u", range_lo);
-	else if (range_lo > 0 || range_hi < 9)
-		sprintf(alias + strlen(alias), "[%u-%u]", range_lo, range_hi);
+		sprintf(alias + strlen(alias), "%X", range_lo);
+	else if (range_lo > 0 || range_hi < max) {
+		if (range_lo > 0x9 || range_hi < 0xA)
+			sprintf(alias + strlen(alias), 
+				"[%X-%X]",
+				range_lo,
+				range_hi);
+		else {
+			sprintf(alias + strlen(alias),
+				range_lo < 0x9 ? "[%X-9" : "[%X",
+				range_lo);
+			sprintf(alias + strlen(alias),
+				range_hi > 0xA ? "a-%X]" : "%X]",
+				range_lo);
+		}
+	}
 	if (bcdDevice_initial_digits < (sizeof(id->bcdDevice_lo) * 2 - 1))
 		strcat(alias, "*");
 
@@ -150,7 +163,7 @@ static void do_usb_entry(struct usb_device_id *id,
 static void do_usb_entry_multi(struct usb_device_id *id, struct module *mod)
 {
 	unsigned int devlo, devhi;
-	unsigned char chi, clo;
+	unsigned char chi, clo, max;
 	int ndigits;
 
 	id->match_flags = TO_NATIVE(id->match_flags);
@@ -162,6 +175,17 @@ static void do_usb_entry_multi(struct usb_device_id *id, struct module *mod)
 	devhi = id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI ?
 		TO_NATIVE(id->bcdDevice_hi) : ~0x0U;
 
+	/* Figure out if this entry is in bcd or hex format */
+	max = 0x9; /* Default to decimal format */
+	for (ndigits = 0 ; ndigits < sizeof(id->bcdDevice_lo) * 2 ; ndigits++) {
+		clo = (devlo >> (ndigits << 2)) & 0xf;
+		chi = ((devhi > 0x9999 ? 0x9999 : devhi) >> (ndigits << 2)) & 0xf;
+		if (clo > max || chi > max) {
+			max = 0xf;
+			break;
+		}
+	}
+
 	/*
 	 * Some modules (visor) have empty slots as placeholder for
 	 * run-time specification that results in catch-all alias
@@ -173,21 +197,21 @@ static void do_usb_entry_multi(struct usb_device_id *id, struct module *mod)
 	for (ndigits = sizeof(id->bcdDevice_lo) * 2 - 1; devlo <= devhi; ndigits--) {
 		clo = devlo & 0xf;
 		chi = devhi & 0xf;
-		if (chi > 9)	/* it's bcd not hex */
-			chi = 9;
+		if (chi > max)	/* If we are in bcd mode, truncate if necessary */
+			chi = max;
 		devlo >>= 4;
 		devhi >>= 4;
 
 		if (devlo == devhi || !ndigits) {
-			do_usb_entry(id, devlo, ndigits, clo, chi, mod);
+			do_usb_entry(id, devlo, ndigits, clo, chi, max, mod);
 			break;
 		}
 
-		if (clo > 0)
-			do_usb_entry(id, devlo++, ndigits, clo, 9, mod);
+		if (clo > 0x0)
+			do_usb_entry(id, devlo++, ndigits, clo, max, max, mod);
 
-		if (chi < 9)
-			do_usb_entry(id, devhi--, ndigits, 0, chi, mod);
+		if (chi < max)
+			do_usb_entry(id, devhi--, ndigits, 0x0, chi, max, mod);
 	}
 }
 

Re: [PATCH] Devices that ignore USB spec generate invalid modaliases

[Greg KH]

On Wed, Nov 11, 2009 at 03:20:23AM -0500, Nathaniel McCallum wrote:
> Please CC me as I'm not subscribed to LKML.
> 
> The current code to generate usb modaliases from usb_device_id assumes 
> that the device's bcdDevice descriptor will actually be in BCD format. 
> While this should be a sane assumption, some devices don't follow spec 
> and just use plain old hex.  This causes drivers for these devices to 
> generate invalid modalias lines which will never actually match for the 
> hardware.
> 
> The following patch adds hex support for bcdDevice in file2alias.c. 
> Drivers for devices which have bcdDevice conforming to BCD will have no 
> change in modalias output.  Drivers for devices which don't conform 
> (primarily usb-storage and ibmcam in my initial survey) should now 
> generate valid modaliases.
> 
> EXAMPLE OUTPUT (ibmcam; space added to highlight change)
> Old: usb:v0545p800D d030[10-9] dc*dsc*dp*ic*isc*ip*
> New: usb:v0545p800D d030a      dc*dsc*dp*ic*isc*ip*

Huh?  The old one had '[]' in it?

What does the bcdDevice for this really look like in the device itself?
If it is messed up in the descriptor, then how can we know to fix it up
here?

And you might want to cc: linux-usb@vger.kernel.org with this as well,
as that's where the USB developers are.

thanks,

greg k-h

Re: [PATCH] Devices that ignore USB spec generate invalid modaliases

[Nathaniel McCallum]

On 11/12/2009 06:42 PM, Greg KH wrote:
> On Wed, Nov 11, 2009 at 03:20:23AM -0500, Nathaniel McCallum wrote:
>> Please CC me as I'm not subscribed to LKML.
>>
>> The current code to generate usb modaliases from usb_device_id assumes
>> that the device's bcdDevice descriptor will actually be in BCD format.
>> While this should be a sane assumption, some devices don't follow spec
>> and just use plain old hex.  This causes drivers for these devices to
>> generate invalid modalias lines which will never actually match for the
>> hardware.
>>
>> The following patch adds hex support for bcdDevice in file2alias.c.
>> Drivers for devices which have bcdDevice conforming to BCD will have no
>> change in modalias output.  Drivers for devices which don't conform
>> (primarily usb-storage and ibmcam in my initial survey) should now
>> generate valid modaliases.
>>
>> EXAMPLE OUTPUT (ibmcam; space added to highlight change)
>> Old: usb:v0545p800D d030[10-9] dc*dsc*dp*ic*isc*ip*
>> New: usb:v0545p800D d030a      dc*dsc*dp*ic*isc*ip*
>
> Huh?  The old one had '[]' in it?
>
> What does the bcdDevice for this really look like in the device itself?
> If it is messed up in the descriptor, then how can we know to fix it up
> here?

The device contains 0x030A (an invalid value since 0x0309 + 1 == 0x0310 
in BCD format) in the bcdDevice descriptor.  The driver matches against 
this descriptor (see the usb_device_id table in 
drivers/media/video/usbvideo/ibmcam.c).

There are three possible solutions:
1. Fix the ibmcam driver not to match against bcdDevice.  I'm not sure 
if this is possible.  Nor does it solve the problem for future devices 
which may be misprogrammed.
2. Change *all* usb modaliases to be generated assuming hex ranges. 
This makes file2alias.c a little simpler at the cost of changing a large 
number of existing modaliases.  The impact of this change is potentially 
large.
3. My solution, which is to detect if either bcdDevice_lo or 
bcdDevice_hi is in hexadecimal rather than BCD format.  We do this by 
checking each character to see if it is above 0x9.  If either of these 
fields is in hex format, we switch into hex mode and render the modalias 
appropriately.  This also has some potential impact on userspace tools, 
however, since currently the only modalias that is affected is ibmcam, 
the change is measurably small in impact.

The second bug (related here: http://lkml.org/lkml/2009/11/11/245) is 
just simply a bug in the modalias generation code.  Namely, 0x59 + 1 == 
0x60 in BCD format, not 0x5A.  Therefore, you cannot rely on i++ to 
increment. Fixing it has no impact other than fixing a broken modalias 
(which occurs in usb-storage).

Nathaniel McCallum

Re: [PATCH] Devices that ignore USB spec generate invalid modaliases

[Nathaniel McCallum]

On 11/12/2009 08:09 PM, Nathaniel McCallum wrote:
> On 11/12/2009 06:42 PM, Greg KH wrote:
>> On Wed, Nov 11, 2009 at 03:20:23AM -0500, Nathaniel McCallum wrote:
>>> Please CC me as I'm not subscribed to LKML.
>>>
>>> The current code to generate usb modaliases from usb_device_id assumes
>>> that the device's bcdDevice descriptor will actually be in BCD format.
>>> While this should be a sane assumption, some devices don't follow spec
>>> and just use plain old hex. This causes drivers for these devices to
>>> generate invalid modalias lines which will never actually match for the
>>> hardware.
>>>
>>> The following patch adds hex support for bcdDevice in file2alias.c.
>>> Drivers for devices which have bcdDevice conforming to BCD will have no
>>> change in modalias output. Drivers for devices which don't conform
>>> (primarily usb-storage and ibmcam in my initial survey) should now
>>> generate valid modaliases.
>>>
>>> EXAMPLE OUTPUT (ibmcam; space added to highlight change)
>>> Old: usb:v0545p800D d030[10-9] dc*dsc*dp*ic*isc*ip*
>>> New: usb:v0545p800D d030a dc*dsc*dp*ic*isc*ip*
>>
>> Huh? The old one had '[]' in it?
>>
>> What does the bcdDevice for this really look like in the device itself?
>> If it is messed up in the descriptor, then how can we know to fix it up
>> here?
>
> The device contains 0x030A (an invalid value since 0x0309 + 1 == 0x0310
> in BCD format) in the bcdDevice descriptor. The driver matches against
> this descriptor (see the usb_device_id table in
> drivers/media/video/usbvideo/ibmcam.c).
>
> There are three possible solutions:
> 1. Fix the ibmcam driver not to match against bcdDevice. I'm not sure if
> this is possible. Nor does it solve the problem for future devices which
> may be misprogrammed.
> 2. Change *all* usb modaliases to be generated assuming hex ranges. This
> makes file2alias.c a little simpler at the cost of changing a large
> number of existing modaliases. The impact of this change is potentially
> large.
> 3. My solution, which is to detect if either bcdDevice_lo or
> bcdDevice_hi is in hexadecimal rather than BCD format. We do this by
> checking each character to see if it is above 0x9. If either of these
> fields is in hex format, we switch into hex mode and render the modalias
> appropriately. This also has some potential impact on userspace tools,
> however, since currently the only modalias that is affected is ibmcam,
> the change is measurably small in impact.
>
> The second bug (related here: http://lkml.org/lkml/2009/11/11/245) is
> just simply a bug in the modalias generation code. Namely, 0x59 + 1 ==
> 0x60 in BCD format, not 0x5A. Therefore, you cannot rely on i++ to
> increment. Fixing it has no impact other than fixing a broken modalias
> (which occurs in usb-storage).

Just in case it is helpful, here is a diff between the old modules.alias 
and the new modules.alias generated with my patches applied.  The 
usb_storage line is fixed by the increment patch.  The ibmcam lines are 
fixed by the bcd/hex patch.

--- modules.alias.old   2009-11-13 09:46:36.191640578 -0500
+++ modules.alias.fixed 2009-11-13 09:46:36.549891821 -0500
@@ -345,7 +345,6 @@
  alias usb:v041Ep4064d*dc*dsc*dp*ic*isc*ip* gspca_ov519
  alias usb:v041Ep4068d*dc*dsc*dp*ic*isc*ip* gspca_ov519
  alias usb:v0420p0001d0100dc*dsc*dp*ic*isc*ip* usb_storage
-alias usb:v0421p0019d05[10-9]*dc*dsc*dp*ic*isc*ip* usb_storage
  alias usb:v0421p0019d059[2-9]dc*dsc*dp*ic*isc*ip* usb_storage
  alias usb:v0421p0019d060*dc*dsc*dp*ic*isc*ip* usb_storage
  alias usb:v0421p0019d0610dc*dsc*dp*ic*isc*ip* usb_storage
@@ -1021,12 +1020,12 @@
  alias usb:v0543p1921d*dc*dsc*dp*ic*isc*ip* ipaq
  alias usb:v0543p1922d*dc*dsc*dp*ic*isc*ip* ipaq
  alias usb:v0543p1923d*dc*dsc*dp*ic*isc*ip* ipaq
-alias usb:v0545p8002d030[10-9]dc*dsc*dp*ic*isc*ip* ibmcam
-alias usb:v0545p800Cd030[10-9]dc*dsc*dp*ic*isc*ip* ibmcam
-alias usb:v0545p800Dd030[10-9]dc*dsc*dp*ic*isc*ip* ibmcam
+alias usb:v0545p8002d030Adc*dsc*dp*ic*isc*ip* ibmcam
+alias usb:v0545p800Cd030Adc*dsc*dp*ic*isc*ip* ibmcam
+alias usb:v0545p800Dd030Adc*dsc*dp*ic*isc*ip* ibmcam
  alias usb:v0545p8080d0002dc*dsc*dp*ic*isc*ip* ibmcam
-alias usb:v0545p8080d030[10-9]dc*dsc*dp*ic*isc*ip* ibmcam
  alias usb:v0545p8080d0301dc*dsc*dp*ic*isc*ip* ibmcam
+alias usb:v0545p8080d030Adc*dsc*dp*ic*isc*ip* ibmcam
  alias usb:v0545p808Bd*dc*dsc*dp*ic*isc*ip* gspca_tv8532
  alias usb:v0545p8333d*dc*dsc*dp*ic*isc*ip* gspca_tv8532
  alias usb:v0546p3155d*dc*dsc*dp*ic*isc*ip* gspca_sunplus