enable usb control message with class specific request

From 724e3b5e8782a584a95d05bb2f44e59743ed3a72 Mon Sep 17 00:00:00 2001

Hi!
Usb devio assumes that the wIndex in every control message apart from those flagged as USB_TYPE_VENDOR holds the number of the Interface being addressed. This is for example not true for the class specific request GET_DEVICE_ID in the printer class:

"The high-byte of the wIndex field is used to specify the zero-based interface index. The low-byte of the wIndex field is used to specify the zero-based alternate setting." [1]

In this special case it misinterpretes the alternate setting 1 for the interface and tries to claim a nonexisting one. Therefor you won't get the printers name.

The patch below is a minimal approach to fix this. Maybe it should be extended to USB_TYPE_RESERVED. Maybe there should be an extended test that knows something about specific classes.

What do you think?
regards Matthias

[1] http://www.usb.org/developers/devclass_docs/usbprint11.pdf

Quoted 15 lines

Hi!
Usb devio assumes that the wIndex in every control message apart from those flagged as USB_TYPE_VENDOR holds the number of the Interface being addressed. This is for example not true for the class specific request GET_DEVICE_ID in the printer class:

"The high-byte of the wIndex field is used to specify the zero-based interface index. The low-byte of the wIndex field is used to specify the zero-based alternate setting." [1]

In this special case it misinterpretes the alternate setting 1 for the interface and tries to claim a nonexisting one. Therefor you won't get the printers name.

Quoted 8 lines

The patch below is a minimal approach to fix this. Maybe it should be extended to USB_TYPE_RESERVED. Maybe there should be an extended test that knows something about specific classes.

What do you think?
regards Matthias

[1] http://www.usb.org/developers/devclass_docs/usbprint11.pdf

In this case, it appears that the printer class specification contradicts the USB-2.0 specification. Section 9.3.1 says (referring to the low-order five bits of bmRequestType):

Requests may be directed to the device, an interface on the device, or a specific endpoint on a device. This field also specifies the intended recipient of the request. When an interface or endpoint is specified, the wIndex field
identifies the interface or endpoint.

And Figure 9-3 shows that when wIndex is used to specify an interface, the interface number belongs in the low-order byte, not the high-order byte.

I don't think it's safe to relax the test the way you have suggested. There are too many other class-specific requests that must be prevented. Maybe an exception could be added for this one particular case. Besides, you don't want to remove the test entirely -- you want to use the high-order byte of wIndex instead of the low-order byte.

The printer spec really is spectacularly bad in this respect. What happens if the printer is a composite device, and the other interface uses the same bmRequestType and bRequest values for its own class-specific purpose, but uses the low-order byte of wIndex to indicate the interface number (as it should). Then the printer wouldn't know which interface was supposed to respond to the message!

Alan Stern

From 6514baecca5e193b78e1f3343585c5c9a458a625 Mon Sep 17 00:00:00 2001

Am Thu, 22 Sep 2011 23:56:56 +0200
schrieb Matthias Dellweg <[email protected]>:

Review

Quoted 5 lines

(quoted diff) +2 -0

+       if ((requesttype == 0xa1) && (request == 0x00)
+        && (usb_find_alt_setting(ps->dev->actconfig, index >> 8,

index & 0xff)

(quoted diff) +2 -0

+        ->desc.bInterfaceClass == USB_CLASS_PRINTER))
+               index >>= 8;

I just woke up and realised this is a very bad idea if there is no such setting...
So here's the next version I'd like to propose for comments:

drivers/usb/core/devio.c +2 -1

index 37518df..d2efc97 100644
@@ -607,9 +607,10 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
 }
 
 static int check_ctrlrecip(struct dev_state *ps, unsigned int requesttype,
-                          unsigned int index)
+                          unsigned int request, unsigned int index)
 {
        int ret = 0;
+       struct usb_host_interface *alt_setting;
 
        if (ps->dev->state != USB_STATE_UNAUTHENTICATED
         && ps->dev->state != USB_STATE_ADDRESS

drivers/usb/core/devio.c +6 -1

+               alt_setting = usb_find_alt_setting(ps->dev->actconfig, index >> 8, index & 0xff);
+               if (alt_setting && (alt_setting->desc.bInterfaceClass == USB_CLASS_PRINTER))
+                       index >>= 8;
+       }
+
        index &= 0xff;
        switch (requesttype & USB_RECIP_MASK) {
        case USB_RECIP_ENDPOINT:

@@ -770,7 +780,7 @@ static int proc_control(struct dev_state *ps, void __user *arg)
 
        if (copy_from_user(&ctrl, arg, sizeof(ctrl)))
                return -EFAULT;
-       ret = check_ctrlrecip(ps, ctrl.bRequestType, ctrl.wIndex);
+       ret = check_ctrlrecip(ps, ctrl.bRequestType, ctrl.bRequest, ctrl.wIndex);
        if (ret)
                return ret;
        wLength = ctrl.wLength;         /* To suppress 64k PAGE_SIZE warning */

This is basically okay. But your mailer has mangled the whitespace. [...]
Alan Stern

From b7c4b6f5780296fd0d53d66e08d5bce0dc450b7f Mon Sep 17 00:00:00 2001

Am Fri, 23 Sep 2011 12:16:40 -0400 (EDT)
schrieb Alan Stern <[email protected]>:

Quoted 3 lines

This is basically okay. But your mailer has mangled the whitespace. [...]
Alan Stern

OK, hoping that my mailer doesn't get wild again, this is the polished version following your suggestions.
Thanks for your help!
Matthias Dellweg

From b7c4b6f5780296fd0d53d66e08d5bce0dc450b7f Mon Sep 17 00:00:00 2001
From: Matthias Dellweg <[email protected]>
Date: Fri, 23 Sep 2011 19:32:39 +0200
Subject: [PATCH] drivers/usb/core/devio.c: Check for printer class specific
request

Signed-off-by Matthias Dellweg <[email protected]>

---
 drivers/usb/core/devio.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

drivers/usb/core/devio.c +16 -2

index 37518df..e4175cc 100644
@@ -607,9 +607,10 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
 }
 
 static int check_ctrlrecip(struct dev_state *ps, unsigned int requesttype,
-			   unsigned int index)
+			   unsigned int request, unsigned int index)
 {
 	int ret = 0;
+	struct usb_host_interface *alt_setting;
 
 	if (ps->dev->state != USB_STATE_UNAUTHENTICATED
 	 && ps->dev->state != USB_STATE_ADDRESS

@@ -618,6 +619,18 @@ static int check_ctrlrecip(struct dev_state *ps, unsigned int requesttype,
 	if (USB_TYPE_VENDOR == (USB_TYPE_MASK & requesttype))
 		return 0;
 
+	/*
+	 * check for the special corner case 'get_device_id' in the printer
+	 * class specification, where wIndex is (interface << 8 | altsetting)
+	 */
+	if (requesttype == 0xa1 && request == 0) {
+		alt_setting = usb_find_alt_setting(ps->dev->actconfig,
+						   index >> 8, index & 0xff);
+		if (alt_setting
+		 && alt_setting->desc.bInterfaceClass == USB_CLASS_PRINTER)
+			index >>= 8;
+	}
+
 	index &= 0xff;
 	switch (requesttype & USB_RECIP_MASK) {
 	case USB_RECIP_ENDPOINT:

@@ -770,7 +783,8 @@ static int proc_control(struct dev_state *ps, void __user *arg)
 
 	if (copy_from_user(&ctrl, arg, sizeof(ctrl)))
 		return -EFAULT;
-	ret = check_ctrlrecip(ps, ctrl.bRequestType, ctrl.wIndex);
+	ret = check_ctrlrecip(ps, ctrl.bRequestType, ctrl.bRequest,
+			      ctrl.wIndex);
 	if (ret)
 		return ret;
 	wLength = ctrl.wLength;		/* To suppress 64k PAGE_SIZE warning */

Quoted 82 lines

Am Fri, 23 Sep 2011 12:16:40 -0400 (EDT)
schrieb Alan Stern <[email protected]>:

Quoted 3 lines

This is basically okay. But your mailer has mangled the whitespace. [...]
Alan Stern

OK, hoping that my mailer doesn't get wild again, this is the polished version following your suggestions.
Thanks for your help!
Matthias Dellweg

From b7c4b6f5780296fd0d53d66e08d5bce0dc450b7f Mon Sep 17 00:00:00 2001 From: Matthias Dellweg <[email protected]>
Date: Fri, 23 Sep 2011 19:32:39 +0200
Subject: [PATCH] drivers/usb/core/devio.c: Check for printer class specific request

Signed-off-by Matthias Dellweg <[email protected]>

---
 drivers/usb/core/devio.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

drivers/usb/core/devio.c +2 -1

index 37518df..e4175cc 100644
@@ -607,9 +607,10 @@ static int findintfep(struct usb_device *dev,
 unsigned int ep) }
 
 static int check_ctrlrecip(struct dev_state *ps, unsigned int
 requesttype,
-			   unsigned int index)
+			   unsigned int request, unsigned int
 index) {
 	int ret = 0;
+	struct usb_host_interface *alt_setting;
 
 	if (ps->dev->state != USB_STATE_UNAUTHENTICATED

drivers/usb/core/devio.c +2 -0

 	 && ps->dev->state != USB_STATE_ADDRESS

@@ -618,6 +619,18 @@ static int check_ctrlrecip(struct dev_state
 *ps, unsigned int requesttype, if (USB_TYPE_VENDOR ==
 (USB_TYPE_MASK & requesttype)) return 0;
 
+	/*
+	 * check for the special corner case 'get_device_id' in

the printer

drivers/usb/core/devio.c +1 -0

+	 * class specification, where wIndex is (interface << 8 |

altsetting)

drivers/usb/core/devio.c +3 -0

+	 */
+	if (requesttype == 0xa1 && request == 0) {
+		alt_setting =

usb_find_alt_setting(ps->dev->actconfig,

drivers/usb/core/devio.c +1 -0

+						   index >> 8,

index & 0xff);

drivers/usb/core/devio.c +2 -0

+		if (alt_setting
+		 && alt_setting->desc.bInterfaceClass ==

USB_CLASS_PRINTER)

drivers/usb/core/devio.c +5 -1

+			index >>= 8;
+	}
+
 	index &= 0xff;
 	switch (requesttype & USB_RECIP_MASK) {
 	case USB_RECIP_ENDPOINT:

@@ -770,7 +783,8 @@ static int proc_control(struct dev_state *ps,
 void __user *arg) 
 	if (copy_from_user(&ctrl, arg, sizeof(ctrl)))
 		return -EFAULT;
-	ret = check_ctrlrecip(ps, ctrl.bRequestType, ctrl.wIndex);
+	ret = check_ctrlrecip(ps, ctrl.bRequestType, ctrl.bRequest,
+			      ctrl.wIndex);
 	if (ret)
 		return ret;
 	wLength = ctrl.wLength;		/* To suppress 64k

PAGE_SIZE warning */ @@ -1100,7 +1114,7 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb, kfree(dr); return -EINVAL;

drivers/usb/core/devio.c +1 -1

 		}
-		ret = check_ctrlrecip(ps, dr->bRequestType,
+		ret = check_ctrlrecip(ps, dr->bRequestType,

dr->bRequest, le16_to_cpup(&dr->wIndex));

drivers/usb/core/devio.c

Quoted 2 lines

drivers/usb/core/devio.c +0 -0

 		if (ret) {
 			kfree(dr);

This looks fine to me. Greg may ask you for a non-empty patch description when you submit it to him.

Acked-by Alan Stern <[email protected]>

Alan Stern

From 5bdb64cbee0b2f353d3bc444c147ba78f34a1f69 Mon Sep 17 00:00:00 2001

From: Matthias Dellweg <[email protected]>

In the usb printer class specific request get_device_id the value of wIndex is (interface << 8 | altsetting) instead of just interface. This enables the detection of some printers with libusb.

The hardware I tested this patch with is a Prolific USB->Centronics cable (067b:2305).

Acked-by Alan Stern <[email protected]>

Signed-off-by Matthias Dellweg <[email protected]>

Cc stable <[email protected]>

---
 drivers/usb/core/devio.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

drivers/usb/core/devio.c +0 -0

index 37518df..1d73709 100644
@@ -607,9 +607,10 @@ static int findintfep(struct usb_device *dev, unsigned int ep)
 }

Quoted 32 lines

From: Matthias Dellweg <[email protected]>

In the usb printer class specific request get_device_id the value of wIndex is (interface << 8 | altsetting) instead of just interface. This enables the detection of some printers with libusb.

The hardware I tested this patch with is a Prolific USB->Centronics cable (067b:2305).

Acked-by Alan Stern <[email protected]>

Signed-off-by Matthias Dellweg <[email protected]>

Cc stable <[email protected]>

---
 drivers/usb/core/devio.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

drivers/usb/core/devio.c +2 -1

index 37518df..1d73709 100644
@@ -607,9 +607,10 @@ static int findintfep(struct usb_device *dev,
 unsigned int ep) }
 
 static int check_ctrlrecip(struct dev_state *ps, unsigned int
 requesttype,
-                          unsigned int index)
+                          unsigned int request, unsigned int index)
 {
        int ret = 0;
+       struct usb_host_interface *alt_setting;
 
        if (ps->dev->state != USB_STATE_UNAUTHENTICATED

Quoted 1 line

         && ps->dev->state != USB_STATE_ADDRESS

All of the tabs were stripped out of this email, making it impossible to apply it :(

Care to fix this up and resend it so that I can apply it?

thanks,

greg k-h

From 5bdb64cbee0b2f353d3bc444c147ba78f34a1f69 Mon Sep 17 00:00:00 2001

Do you accept it as an attachment?