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 <[email protected]>
Cologne, Germany
Hi Alan,
thanks for your review and your constructive comments!
Alan Stern <[email protected]> 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 <[email protected]>
>> Date: Sun Sep 22 11:54:59 2013 +0200
>>
>> From: Kurt Garloff <[email protected]>
>> Subject: Tolerate wrong direction bit endpoint index for control messages
>> Signed-off-by: Kurt Garloff <[email protected]>
>>
>> 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 00000000 0000000001a83420 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 0000000001b56f00, 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 kernel: [44539.730301] usb 4-2.2: proc_submiturb: return 0
The second line here comes from instrumentation I have inserted in proc_submiturb():
snoop(&ps->dev->dev, "%s: URB %02x %02x %i %08x %p %i %i\n",
__func__, uurb.type, uurb.endpoint, uurb.status,
uurb.flags, uurb.buffer, uurb.buffer_length,
uurb.actual_length);
and it shows that uurb.endpoint actually is set to 0.
>> 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?
>
>No, Linux is correct. While it is true that the direction bit is
>ignored for control endpoints, in this case we are talking about
>endpoint 1, which is not a control endpoint. In an HID device, it is
>almost certainly an interrupt endpoint.
Indeed, the device has EPs 0x81 and 0x82, both of which are
interrupt EPs.
>> 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.
>
>It applies to all devices, but it isn't relevant here.
>
>> - 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.
>
>Of course there can be. It is entirely possible to have one endpoint
>with address 0x01 and another with address 0x81.
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.
(And again the behavior might not be enforced by the spec, but maybe
by Windows?)
>However, your patch doesn't run into this problem. If both endpoints
>exist, the routine will use the one with the address given by wIndex
>and ignore the other -- your new code won't run at all.
Yep.
>> - 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 */
>
> /*
> * The accepted format for multiline comments
> * looks like this.
> */
OK, thanks for pointing this out!
>> + 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",
>
>Why do you have the "(or 00)" part? It doesn't seem to make any sense.
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.
>> + __func__,
>> + task_pid_nr(current),
>> + current->comm,
>> + index, index ^ 0x80);
>> + }
>> if (ret >= 0)
>> ret = checkintf(ps, ret);
>> break;
Thanks for the review! I will submit a new patch.
--
Kurt Garloff <[email protected]>
On Sun, 22 Sep 2013, Kurt Garloff wrote:
> Hi Alan,
>
> thanks for your review and your constructive comments!
You're welcome.
> >> 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.
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.
> >> 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.)
This is a good explanation of why the patch should be accepted.
> >> From: Kurt Garloff <[email protected]>
> >> Subject: Tolerate wrong direction bit endpoint index for control messages
> >> Signed-off-by: Kurt Garloff <[email protected]>
> >>
> >> 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?
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?
> 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 00000000 0000000001a83420 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 0000000001b56f00, 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 kernel: [44539.730301] usb 4-2.2: proc_submiturb: return 0
>
> The second line here comes from instrumentation I have inserted in proc_submiturb():
>
> snoop(&ps->dev->dev, "%s: URB %02x %02x %i %08x %p %i %i\n",
> __func__, uurb.type, uurb.endpoint, uurb.status,
> uurb.flags, uurb.buffer, uurb.buffer_length,
> uurb.actual_length);
>
> and it shows that uurb.endpoint actually is set to 0.
That's right. The URB is sent to endpoint 0. The value in
bRequestType indicates that the recipient of the request is an
endpoint, and the value of wIndex indicates which endpoint. So even
though the request is sent to endpoint 0, it actually refers to
endpoint 0x01 (or 0x81, as the case may be).
As an analogy, consider the Clear-Halt request. This request is sent
to endpoint 0, and it tells the device to clear the Halt feature for
some other endpoint -- that other endpoint is specified in wIndex.
> >> - We don't risk sending to a wrong endpoint, as there can't be two
> >> endpoints with same index and just different direction.
> >
> >Of course there can be. It is entirely possible to have one endpoint
> >with address 0x01 and another with address 0x81.
>
> 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.
> >> + dev_info(&ps->dev->dev ,
> >> + "%s: process %i (%s) requesting ep %02x but needs %02x (or
> >00)\n",
> >
> >Why do you have the "(or 00)" part? It doesn't seem to make any sense.
>
> 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.
> Thanks for the review! I will submit a new patch.
Good.
Alan Stern
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 <[email protected]>
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 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)
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 <[email protected]>
Cc: [email protected]
---
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 *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
+ * ndex wrong and have the endpoint number here
+ * rather than the endpoint address (with the
+ * correct direction). Win does let this through,
+ * so we'll give it a second try as well (to not
+ * break KVM) -- but 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 <[email protected]>
Cologne, Germany
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.
> >> (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).
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. 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.
> 8<--------
>
> From: Kurt Garloff <[email protected]>
> 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.
> 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.
> 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 <[email protected]>
> Cc: [email protected]
Fix the spelling (.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..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 *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
> + * ndex wrong and have the endpoint number here
s/ndex/index/
> + * rather than the endpoint address (with the
> + * correct direction). Win does let this through,
> + * so we'll give it a second try as well (to not
> + * break KVM) -- but 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;
After you make these changes, you can add:
Acked-by: Alan Stern <[email protected]>
Alan Stern
Hi Alan,
Alan Stern <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> > Cc: [email protected]
>
> 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 *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
> > + * ndex wrong and have the endpoint number here
>
> s/ndex/index/
Thx.
>
> > + * rather than the endpoint address (with the
> > + * correct direction). Win does let this through,
> > + * so we'll give it a second try as well (to not
> > + * break KVM) -- but 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;
>
> After you make these changes, you can add:
>
> Acked-by: Alan Stern <[email protected]>
Thank you, will do that!
And thanks for all your support.
--
Kurt Garloff <[email protected]>
(Sent from Android Phone with K-9 Mail.)