[PATCH] usb/core/devio.c: Don't reject control message to endpoint with wrong direction bit
Hi,
this has been discussed on linux-usb and Alan Stern provided very
helpful feedback.
Please merge this patch ...
From: Kurt Garloff
Date: Mon, 23 Sep 2013 14:19:02 +0200
Subject: Tolerate wrong direction bit in endpoint address for control messages
Trying to read data from the Pegasus Technologies NoteTaker (0e20:0101)
[1] with the Windows App (EasyNote) works natively but fails when
Windows is running under KVM (and the USB device handed to KVM).
The reason is a USB control message
usb 4-2.2: control urb: bRequestType=22 bRequest=09 wValue=0200 wIndex=0001
wLength=0008
This goes to endpoint address 0x01 (wIndex); however, endpoint address
0x01 does not exist. There is an endpoint 0x81 though (same number,
but other direction); the app may have meant that endpoint instead.
The kernel thus rejects the IO and thus we see the failure.
Apparently, Linux is more strict here than Windows ... we can't change
the Win app easily, so that's a problem.
It seems that the Win app/driver is buggy here and the driver does not
behave fully according to the USB HID class spec that it claims to
belong to. The device seems to happily deal with that though (and
seems to not really care about this value much).
So the question is whether the Linux kernel should filter here.
Rejecting has the risk that somewhat non-compliant userspace apps/
drivers (most likely in a virtual machine) are prevented from working.
Not rejecting has the risk of confusing an overly sensitive device with
such a transfer. Given the fact that Windows does not filter it makes
this risk rather small though.
The patch makes the kernel more tolerant: If the endpoint address in
wIndex does not exist, but an endpoint with toggled direction bit does,
it will let the transfer through. (It does NOT change the message.)
With attached patch, the app in Windows in KVM works.
usb 4-2.2: check_ctrlrecip: process 13073 (qemu-kvm) requesting ep 01 but
needs 81
I suspect this will mostly affect apps in virtual environments; as on
Linux the apps would have been adapted to the stricter handling of the
kernel. I have done that for mine[2].
[1] http://www.pegatech.com/
[2] https://sourceforge.net/projects/notetakerpen/
Signed-off-by: Kurt Garloff
Acked-by: Alan Stern
Cc: sta...@vger.kernel.org
---
drivers/usb/core/devio.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 737e3c1..71dc5d7 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -742,6 +742,22 @@ static int check_ctrlrecip(struct dev_state *ps, unsigned
int requesttype,
if ((index & ~USB_DIR_IN) == 0)
return 0;
ret = findintfep(ps->dev, index);
+ if (ret < 0) {
+ /*
+* Some not fully compliant Win apps seem to get
+* index wrong and have the endpoint number here
+* rather than the endpoint address (with the
+* correct direction). Win does let this through,
+* so we'll not reject it here but leave it to
+* the device to not break KVM. But we warn.
+*/
+ ret = findintfep(ps->dev, index ^ 0x80);
+ if (ret >= 0)
+ dev_info(&ps->dev->dev,
+ "%s: process %i (%s) requesting ep %02x
but needs %02x\n",
+ __func__, task_pid_nr(current),
+ current->comm, index, index ^ 0x80);
+ }
if (ret >= 0)
ret = checkintf(ps, ret);
break;
--
Kurt Garloff [Koeln, Germany]
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb/core/devio.c:tolerate wrong direction flag in control endpoints
Hi Alan, Alan Stern schrieb: > On Mon, 23 Sep 2013, Kurt Garloff wrote: > > > >> that qualifies as a bug or not. Maybe it should not claim to be a > > >> HID device then? > > > Maybe not. This particular combination of bRequestType and > bRequest > > > values (0x22, 0x09) is not defined in the HID 1.11 spec. Do you > know > > > if it is defined somewhere else? > > These are custom commands, somewhat described at > > > http://pegatech.com/_Uploads/Downloads/Documents/Protocol_Definition_Rev_1.12.pdf > > That document describes a UART protocol with no mention of USB at all. Yep, probably they just took the serial protocol to USB... Beyond the spec, I did watch the Win app a bit when implementing [2] ... > > With behavior here I referred to the fact that I have not yet seen a > USB > > device that > > has two endpoints with the same endpoint number (but different direction). > > I have. They aren't very common but they do exist. > > > Let me try inline insert (by c'n'p: I switched from mutt to Thunderbird > > recently and lack > > experience whether this breaks formatting or so ...) > > It did mangle the whitespace characters. Cr*p. > That doesn't matter for > reviewing, but it is important when you submit the patch. Take a look > > at Documentation/email-clients.txt for some suggestions. I'll go back to mutt then, I guess - used it for >10 years... > > 8< > > > > From: Kurt Garloff > > Date: Mon, 23 Sep 2013 14:19:02 +0200 > > Subject: Tolerate wrong direction bit in endpoint address for control > > messages > > > > Trying to read data from the Pegasus Technologies NoteTaker (0e20:0101) > > [1] with the Windows App (EasyNote) works natively but fails when > > Windows is running under KVM (and the USB device handed to KVM). > > > > The reason is a USB control message > > usb 4-2.2: control urb: bRequestType=22 bRequest=09 wValue=0200 > > wIndex=0001 wLength=0008 > > This goes to endpoint address 0x01 (wIndex); however, endpoint number 1 > > is an input endpoint and thus has endpoint address 0x81. > > You should say something like: > > however, endpoint 0x01 doesn't exist. There is an endpoint > 0x81, though; perhaps the app meant that endpoint instead. OK. > > The kernel thus rejects the IO and thus we see the failure. > > > > Apparently, Linux is more strict here than Windows ... we can't change > > the Win app easily, so that's a problem. > > > > It seems that the Win app/driver is buggy here and the driver does not > > behave fully according to the USB HID class that it claims to belong to. > > The device seems to happily deal with that though (and seems to not > > really care about this value much). > > > > So the question is whether the Linux kernel should filter here. > > Rejecting has the risk that somewhat non-compliant userspace apps/ > > drivers (most likely in a virtual machine) are prevented from > working. > > Not rejecting has the risk of confusing an overly sensitive device with > > such a transfer. Given the fact that Windows does not filter it makes > > this risk rather small though. > > > > The patch makes the kernel more tolerant: If the endpoint address in > > wIndex does not exist, but an endpoint with toggled direction bit does, > > it will let the transfer through. (It does NOT change the message.) > > > > With attached patch, the app in Windows in KVM works. > > usb 4-2.2: check_ctrlrecip: process 13073 (qemu-kvm) requesting ep > 01 > > but needs 81 (or 00) > > You need to remove the "(or 00)" here. Good catch! I changed the code but not the log... > > I suspect this will mostly affect apps in virtual environments; as on > > Linux the apps would have been adapted to the stricter handling of the > > kernel. I have done that for mine[2]. > > > > [1] http://www.pegatech.com/ > > [2] https://sourceforge.net/projects/notetakerpen/ > > > > Signed-off-by: Kurt Garloff > > Cc: sta...@vger.kernel.or > > Fix the spelling (.org). The second time in two days, my fingers are getting too slow... > > --- > > drivers/usb/core/devio.c | 16 > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > > index 737e3c1..4ff61f9 100644 > > --- a/drivers/usb/core/devio.c > > +++ b/drivers/usb/core/devio.c > > @@ -742,6 +742,22 @@ static int check_ctrlrecip(struct dev_state *
Re: [PATCH] usb/core/devio.c:tolerate wrong direction flag in control endpoints
Hi Alan, On 09/23/2013 04:28 AM, Alan Stern wrote: > On Sun, 22 Sep 2013, Kurt Garloff wrote: > >> Well, this seems to be a question of terminology, no? >> I saw the endpoint byte as consisting of endpoint index plus the direction >> bit. > See the entry for "Endpoint Address" in Chapter 2 (Terms and > Abbreviations) of the USB 2.0 specification. The definition says: > > The combination of an endpoint number and an endpoint direction > on a USB device. Each endpoint address supports data transfer > in one direction. OK. This definitely helps me to use the correct terminology. >> OK, you certainly know the USB specs better than I do. >> >> If the message is not according to spec because the windex byte (which >> should reference the endpoint) has the endpoint direction flag wrong, >> then the question may become whether the kernel should reject it or >> leave that to the device. >> Rejecting by the kernel has the risk that somewhat non-compliant devices >> with somewhat non-compliant (userspace) drivers are prevented from working. >> While not rejecting has the risk that overly sensitive devices might freak >> out >> on receiving a non-compliant transfer. The fact that Win does not not seem to >> filter here however might make that risk rather small. >> (Long years have taught us that companies don't test against the spec but >> this >> "OS" from Redmond.) > This is a good explanation of why the patch should be accepted. OK, I added it into the patch header. >> It seems to interpret wIndex differently indeed. Not sure whether >> that qualifies as a bug or not. Maybe it should not claim to be a >> HID device then? > Maybe not. This particular combination of bRequestType and bRequest > values (0x22, 0x09) is not defined in the HID 1.11 spec. Do you know > if it is defined somewhere else? These are custom commands, somewhat described at http://pegatech.com/_Uploads/Downloads/Documents/Protocol_Definition_Rev_1.12.pdf >> Hmmm, none of the devices I have show this. >> My impression was that the EP byte is composed of >> ep = epindex | (dir==in? 0x80: 0) >> and that index alone must be unique already. But then again, I'm in no >> way an expert in USB specs and may just have jumped to conclusions >> from the wording. > The spec is pretty clear about this. For example, it says that in > addition to endpoint 0, a device can have up to 15 input endpoints and > up to 15 output endpoints (section 5.3.1.2). > >> (And again the behavior might not be enforced by the spec, but maybe >> by Windows?) > More likely the behavior isn't enforced at all. The device may simply > be buggy. With behavior here I referred to the fact that I have not yet seen a USB device that has two endpoints with the same endpoint number (but different direction). Anyway, that's not relevant to the patch ... We don't change the value of wIndex, we just decide to let the transfer through and let the device deal with it. >> Based on the observation that uurb.endpoint = 0 (see above), I have >> changed my code in the Linux program (at [2]) to use 0x00 as wIndex >> instead of 0x81 (or formerly 0x01). It still worked. >> So it's questionable, whether wIndex should even point to an EP ... >> and the hardware might just ignore it. > It looks that way. The request claims to be class-specific, so it > would be nice to know which class document defines the request's > format. I guess none ... I just dropped the (or 00), as it's not reflected in the code ... >> Thanks for the review! I will submit a new patch. > Good. Find it here. (Let me know if it should be sent again separately for some reason.) Let me try inline insert (by c'n'p: I switched from mutt to Thunderbird recently and lack experience whether this breaks formatting or so ...) 8< From: Kurt Garloff Date: Mon, 23 Sep 2013 14:19:02 +0200 Subject: Tolerate wrong direction bit in endpoint address for control messages Trying to read data from the Pegasus Technologies NoteTaker (0e20:0101) [1] with the Windows App (EasyNote) works natively but fails when Windows is running under KVM (and the USB device handed to KVM). The reason is a USB control message usb 4-2.2: control urb: bRequestType=22 bRequest=09 wValue=0200 wIndex=0001 wLength=0008 This goes to endpoint address 0x01 (wIndex); however, endpoint number 1 is an input endpoint and thus has endpoint address 0x81. The kernel thus rejects the IO and thus we see the failure. Apparently, Linux is more strict here than Windows ... we can't change the Win app easily, so that's a problem. It seems that the Win app/driver is buggy he
Re: [PATCH] usb/core/devio.c:tolerate wrong direction flag in control endpoints
Hi Alan, thanks for your review and your constructive comments! Alan Stern schrieb: >On Sun, 22 Sep 2013, Kurt Garloff wrote: > >> Hi, >> >> USB devio rejects control messages when the index does not have the >> direction bit set correctly. > >I wouldn't describe it that way. It would be more accurate to say that >the message is rejected when wIndex contains an invalid endpoint >address. Well, this seems to be a question of terminology, no? I saw the endpoint byte as consisting of endpoint index plus the direction bit. >> This breaks windows apps in KVM -- and might be overly strict >according >> to my reading of USB HID spec. > >It is not overly strict. OK, you certainly know the USB specs better than I do. If the message is not according to spec because the windex byte (which should reference the endpoint) has the endpoint direction flag wrong, then the question may become whether the kernel should reject it or leave that to the device. Rejecting by the kernel has the risk that somewhat non-compliant devices with somewhat non-compliant (userspace) drivers are prevented from working. While not rejecting has the risk that overly sensitive devices might freak out on receiving a non-compliant transfer. The fact that Win does not not seem to filter here however might make that risk rather small. (Long years have taught us that companies don't test against the spec but this "OS" from Redmond.) >> Attached patch makes the kernel tolerant against it and makes the app >> work for me. >> >> More details in the patch header. >> >> USB experts: Please review this and judge whether this is correct, >> applies more generically, >> or maybe needs to be special cased (only for USB HID devices?) or >> implemented as quirk >> or module/kernel parameter. > >The patch seems reasonable enough, although the description needs >improvement and a couple of minor things should be fixed. More >comments below. > >In the future, please put patches inline with the rest of the email >message. Don't make them attachments unless you have to; that way they >become harder to read and comment on. Seems I have not sent a patch for too long, so I failed to remember this longstanding rule when sending. Sorry for this! >> Once in the final form, this *might* be stable material. > >Yes, it should be. > >> commit bc1e4e1ae1d5a4f9b2d263f22c651dd5ba4f8ff9 >> Author: Kurt Garloff >> Date: Sun Sep 22 11:54:59 2013 +0200 >> >> From: Kurt Garloff >> Subject: Tolerate wrong direction bit endpoint index for control messages >> Signed-off-by: Kurt Garloff >> >> Trying to read data from the Pegasus Technologies NoteTaker (0e20:0101) >> [1] with the Windows App (EasyNote) works natively but fails when >> WIndows is running under KVM (and the USB device handed to KVM). >> >> The reason is a USB control message >> usb 4-2.2: control urb: bRequestType=22 bRequest=09 wValue=0200 >> wIndex=0001 wLength=0008 >> This goes to endpoint 1 (wIndex), however, the endpoint is an input >> endpoint and thus enumerated 0x81. >> >> The kernel thus rejects the IO and thus we see the failure. >> >> Apparently, Linux is more strict here than Windows ... we can't change >> the Win app easily, so that's a problem. > >Indeed, this indicates that the device itself is also buggy. If it >worked correctly, it would reject the incorrect control message. It seems to interpret wIndex differently indeed. Not sure whether that qualifies as a bug or not. Maybe it should not claim to be a HID device then? Looking at the code, it seems that printers do something strange here, and it might be that the device in question here is not 100% HID in that it also assumes a non-standard meaning to this byte. Strange enough, the app does want to talk to the control interface it seems: Sep 19 09:47:21 nehalem kernel: [44539.730269] usb 4-2.2: usbdev_do_ioctl: SUBMITURB Sep 19 09:47:21 nehalem kernel: [44539.730276] usb 4-2.2: proc_submiturb: URB 02 00 0 01a83420 16 0 Sep 19 09:47:21 nehalem kernel: [44539.730280] usb 4-2.2: control urb: bRequestType=22 bRequest=09 wValue=0200 wIndex=0001 wLength=0008 Sep 19 09:47:21 nehalem kernel: [44539.730285] usb 4-2.2: check_ctrlrecip: process 13073 (qemu-kvm) requesting ep 01 but needs 81 (or 00) Sep 19 09:47:21 nehalem kernel: [44539.730290] usb 4-2.2: userurb 01b56f00, ep0 ctrl-out, length 8 Sep 19 09:47:21 nehalem kernel: [44539.730294] data: 02 01 b5 00 00 00 00 00 Sep 19 09:47:21 nehalem ker
[PATCH] usb/core/devio.c:tolerate wrong direction flag in control endpoints
Hi,
USB devio rejects control messages when the index does not have the
direction bit set correctly.
This breaks windows apps in KVM -- and might be overly strict according
to my reading of USB HID spec.
Attached patch makes the kernel tolerant against it and makes the app
work for me.
More details in the patch header.
USB experts: Please review this and judge whether this is correct,
applies more generically,
or maybe needs to be special cased (only for USB HID devices?) or
implemented as quirk
or module/kernel parameter.
Once in the final form, this *might* be stable material.
Please keep me in copy for the discussion, my participation on LKML is
mostly reading summaries
from Jonathan and Thorsten these days, unfortunately.
--
Kurt Garloff
Cologne, Germany
commit bc1e4e1ae1d5a4f9b2d263f22c651dd5ba4f8ff9
Author: Kurt Garloff
Date: Sun Sep 22 11:54:59 2013 +0200
From: Kurt Garloff
Subject: Tolerate wrong direction bit endpoint index for control messages
Signed-off-by: Kurt Garloff
Trying to read data from the Pegasus Technologies NoteTaker (0e20:0101)
[1] with the Windows App (EasyNote) works natively but fails when
WIndows is running under KVM (and the USB device handed to KVM).
The reason is a USB control message
usb 4-2.2: control urb: bRequestType=22 bRequest=09 wValue=0200 wIndex=0001 wLength=0008
This goes to endpoint 1 (wIndex), however, the endpoint is an input
endpoint and thus enumerated 0x81.
The kernel thus rejects the IO and thus we see the failure.
Apparently, Linux is more strict here than Windows ... we can't change
the Win app easily, so that's a problem.
However, the app might not even be buggy here. Browsing the USB HID
spec, there's a note that the bit for direction is ignored for control
endpoints ... so Linux might be overly strict?
With attached patch, everything works.
usb 4-2.2: check_ctrlrecip: process 13073 (qemu-kvm) requesting ep 01 but needs 81 (or 00)
Notes:
- I have not checked whether the ignorance of the direction bit for
control endpoints only applies to USB HID devices, thus I have not
special-cased depending on the device type.
- We do output a warning into the kernel log, as this might still be
caused by an application error.
- We don't risk sending to a wrong endpoint, as there can't be two
endpoints with same index and just different direction.
- I suspect this will mostly affect apps in virtual environments; as on
Linux the apps would have been adapted to the stricter handling of the
kernel. I have done that for mine[2], although delaying the release by
two years
[1} http://www.pegatech.com/
[2] https://sourceforge.net/projects/notetakerpen/
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index c2f62a3..8acbc2f 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -623,6 +623,19 @@ static int check_ctrlrecip(struct dev_state *ps, unsigned int requesttype,
switch (requesttype & USB_RECIP_MASK) {
case USB_RECIP_ENDPOINT:
ret = findintfep(ps->dev, index);
+ if (ret < 0) {
+ /* OK, give it a second try -- user might have confused
+ * direction -- this happens from virtualized win apps
+ * e.g. -- warn, but be graceful */
+ ret = findintfep(ps->dev, index ^ 0x80);
+ if (ret >= 0)
+dev_info(&ps->dev->dev ,
+ "%s: process %i (%s) requesting ep %02x but needs %02x (or 00)\n",
+ __func__,
+ task_pid_nr(current),
+ current->comm,
+ index, index ^ 0x80);
+ }
if (ret >= 0)
ret = checkintf(ps, ret);
break;
