bug in handling of highspeed usb HID devices

bug in handling of highspeed usb HID devices

[Christian Krause]

Hi folks,

During the development of an USB device I found a bug in the handling of
Highspeed HID devices in the kernel.

What happened?

Highspeed HID devices are correctly recognized and enumerated by the
kernel. But even if usbhid kernel module is loaded, no HID reports are
received by the kernel.

The output of the hardware USB analyzer told me that the host doesn't
even poll for interrupt IN transfers (even the "interrupt in" USB
transfer are polled by the host).


After some debugging in hid-core.c I've found the reason.

In case of a highspeed device, the endpoint interval is re-calculated in
driver/usb/input/hid-core.c:

line 1669:
             /* handle potential highspeed HID correctly */
             interval = endpoint->bInterval;
             if (dev->speed == USB_SPEED_HIGH)
                   interval = 1 << (interval - 1);

Basically this calculation is correct (refer to USB 2.0 spec, 9.6.6).
This new calculated value of "interval" is used as input for
usb_fill_int_urb:

line 1685:

            usb_fill_int_urb(hid->urbin, dev, pipe, hid->inbuf, 0,
                   hid_irq_in, hid, interval);

Unfortunately the same calculation as above is done a second time in 
usb_fill_int_urb in the file include/linux/usb.h:

line 933:
        if (dev->speed == USB_SPEED_HIGH)
                urb->interval = 1 << (interval - 1);
        else
                urb->interval = interval;

This means, that if the endpoint descriptor (of a high speed device)
specifies e.g. bInterval = 7, the urb->interval gets the value:

hid-core.c: interval = 1 << (7-1) = 0x40 = 64
urb->interval = 1 << (interval -1) = 1 << (63) = integer overflow

Because of this the value of urb->interval is sometimes negative and is
rejected in core/urb.c:
line 353:
                /* too small? */
                if (urb->interval <= 0)
                        return -EINVAL;


The conclusion is, that the recalculaton of the interval (which is
necessary for highspeed) should not be made twice, because this is
simply wrong. ;-)


Re-calculation in usb_fill_int_urb makes more sense, because it is the
most general approach. So it would make sense to remove it from
hid-core.c.


Because in hid-core.c the interval variable is only used for calling
usb_fill_int_urb, it is no problem to remove the highspeed
re-calculation in this file.

Here is a small patch which solves the whole problem:

--------------------------
--- hid-core.c.old      2005-10-12 21:29:29.000000000 +0200
+++ hid-core.c  2005-10-12 21:31:02.000000000 +0200
@@ -1667,11 +1667,6 @@
                if ((endpoint->bmAttributes & 3) != 3)          /* Not an interrupt endpoint */
                        continue;
 
-               /* handle potential highspeed HID correctly */
-               interval = endpoint->bInterval;
-               if (dev->speed == USB_SPEED_HIGH)
-                       interval = 1 << (interval - 1);
-
                /* Change the polling interval of mice. */
                if (hid->collection->usage == HID_GD_MOUSE && hid_mousepoll_interval > 0)
                        interval = hid_mousepoll_interval;

----------------------------

Please review my investigation and if you come to the same conclusion
please apply this patch in the next kernel versions. If not, please tell
me why. ;-)


Best regards,
Christian

Re: bug in handling of highspeed usb HID devices

[Greg KH]

On Wed, Oct 12, 2005 at 09:55:32PM +0200, Christian Krause wrote:
> 
> Here is a small patch which solves the whole problem:
> 
> --------------------------
> --- hid-core.c.old      2005-10-12 21:29:29.000000000 +0200
> +++ hid-core.c  2005-10-12 21:31:02.000000000 +0200

The patch is at the wrong level, and has spaces instead of tabs.

And no "signed-off-by" line :(

Take a look at Documentation/SubmittingPatches for how to create a patch
that I can apply and forward on.

Also, what device needs this patch?  Is it a device that I can buy
today?

thanks,

greg k-h

[PATCH] Re: bug in handling of highspeed usb HID devices

[Christian Krause]

Hi Greg,

On Thu, 13 Oct 2005 15:48:39 -0700, Greg KH wrote:
> On Wed, Oct 12, 2005 at 09:55:32PM +0200, Christian Krause wrote:
>> Here is a small patch which solves the whole problem:

> The patch is at the wrong level, and has spaces instead of tabs.
> And no "signed-off-by" line :(
> Take a look at Documentation/SubmittingPatches for how to create a patch
> that I can apply and forward on.

Ok, next try with Signed-off-by above the patch. Please apologize the
spam. ;-)

During the development of an USB device I found a bug in the handling of
Highspeed HID devices in the kernel.

What happened?

Highspeed HID devices are correctly recognized and enumerated by the
kernel. But even if usbhid kernel module is loaded, no HID reports are
received by the kernel.

The output of the hardware USB analyzer told me that the host doesn't
even poll for interrupt IN transfers (even the "interrupt in" USB
transfer are polled by the host).

After some debugging in hid-core.c I've found the reason.

In case of a highspeed device, the endpoint interval is re-calculated in
driver/usb/input/hid-core.c:

line 1669:
             /* handle potential highspeed HID correctly */
             interval = endpoint->bInterval;
             if (dev->speed == USB_SPEED_HIGH)
                   interval = 1 << (interval - 1);

Basically this calculation is correct (refer to USB 2.0 spec, 9.6.6).
This new calculated value of "interval" is used as input for
usb_fill_int_urb:

line 1685:

            usb_fill_int_urb(hid->urbin, dev, pipe, hid->inbuf, 0,
                   hid_irq_in, hid, interval);

Unfortunately the same calculation as above is done a second time in 
usb_fill_int_urb in the file include/linux/usb.h:

line 933:
        if (dev->speed == USB_SPEED_HIGH)
                urb->interval = 1 << (interval - 1);
        else
                urb->interval = interval;

This means, that if the endpoint descriptor (of a high speed device)
specifies e.g. bInterval = 7, the urb->interval gets the value:

hid-core.c: interval = 1 << (7-1) = 0x40 = 64
urb->interval = 1 << (interval -1) = 1 << (63) = integer overflow

Because of this the value of urb->interval is sometimes negative and is
rejected in core/urb.c:
line 353:
                /* too small? */
                if (urb->interval <= 0)
                        return -EINVAL;

The conclusion is, that the recalculaton of the interval (which is
necessary for highspeed) should not be made twice, because this is
simply wrong. ;-)

Re-calculation in usb_fill_int_urb makes more sense, because it is the
most general approach. So it would make sense to remove it from
hid-core.c.

Because in hid-core.c the interval variable is only used for calling
usb_fill_int_urb, it is no problem to remove the highspeed
re-calculation in this file.

Signed-off-by: Christian Krause <chkr@plauener.de>

--------------------------------snip------------------------
--- linux-2.6.13.4/drivers/usb/input/hid-core.c.old	2005-10-12 21:29:29.000000000 +0200
+++ linux-2.6.13.4/drivers/usb/input/hid-core.c	2005-10-12 21:31:02.000000000 +0200
@@ -1667,11 +1667,6 @@ static struct hid_device *usb_hid_config
 		if ((endpoint->bmAttributes & 3) != 3)		/* Not an interrupt endpoint */
 			continue;
 
-		/* handle potential highspeed HID correctly */
-		interval = endpoint->bInterval;
-		if (dev->speed == USB_SPEED_HIGH)
-			interval = 1 << (interval - 1);
-
 		/* Change the polling interval of mice. */
 		if (hid->collection->usage == HID_GD_MOUSE && hid_mousepoll_interval > 0)
 			interval = hid_mousepoll_interval;

--------------------------------snip------------------------


Best regards,
Christian

Re: [PATCH] Re: bug in handling of highspeed usb HID devices

[Greg KH]

On Fri, Oct 14, 2005 at 07:57:45PM +0200, Christian Krause wrote:
> --- linux-2.6.13.4/drivers/usb/input/hid-core.c.old	2005-10-12 21:29:29.000000000 +0200
> +++ linux-2.6.13.4/drivers/usb/input/hid-core.c	2005-10-12 21:31:02.000000000 +0200
> @@ -1667,11 +1667,6 @@ static struct hid_device *usb_hid_config
>  		if ((endpoint->bmAttributes & 3) != 3)		/* Not an interrupt endpoint */
>  			continue;
>  
> -		/* handle potential highspeed HID correctly */
> -		interval = endpoint->bInterval;
> -		if (dev->speed == USB_SPEED_HIGH)
> -			interval = 1 << (interval - 1);
> -

Did you try this patch out?  It is wrong.  Please look at the compiler
warning that this change generates and redo the patch.

thanks,

greg k-h

Re: [PATCH] Re: bug in handling of highspeed usb HID devices

[Parag Warudkar]

On Friday 14 October 2005 19:42, Greg KH wrote:
> Did you try this patch out?  It is wrong.  Please look at the
> compiler warning that this change generates and redo the patch.

[CC'ed Andrew - likely he has the wrong patch queued up.]

And to save Christian some time, here is a hint - interval is used 
uninitialized!

Parag

Re: [PATCH] Re: bug in handling of highspeed usb HID devices

[Greg KH]

On Fri, Oct 14, 2005 at 09:43:21PM -0400, Parag Warudkar wrote:
> On Friday 14 October 2005 19:42, Greg KH wrote:
> > Did you try this patch out? ?It is wrong. ?Please look at the
> > compiler warning that this change generates and redo the patch.
> 
> [CC'ed Andrew - likely he has the wrong patch queued up.]

Andrew's already dropped the wrong patch.

> And to save Christian some time, here is a hint - interval is used 
> uninitialized!

Which leads me to believe that Christian never tried the patch :(

thanks,

greg k-h

[PATCH] Re: bug in handling of highspeed usb HID devices

[Christian Krause]

Hi Greg,

On Fri, 14 Oct 2005 19:18:18 -0700, Greg KH wrote:
>> > Did you try this patch out? ?It is wrong. ?Please look at the
>> > compiler warning that this change generates and redo the patch.

> Which leads me to believe that Christian never tried the patch :(

I'm very sorry, it seems that my first linux kernel patch will be a real
desaster. 

Unfortunately, Greg, you are right. To get a real clean patch without my
debug printks I've made my changes again on a clean kernel tree and so I
missed a line and I did not recompile the kernel again before submitting
the patch. I'm mortified, but I've learned the lesson - it will never
happen again. Here is the next try:

This patch compiles cleanly on a vanilla kernel 2.6.13.4 and I've tested
that it fixes the problem - the interval for highspeed HID devices is
correctly set.

explanation and patch:



During the development of an USB device I found a bug in the handling of
Highspeed HID devices in the kernel.

What happened?

Highspeed HID devices are correctly recognized and enumerated by the
kernel. But even if usbhid kernel module is loaded, no HID reports are
received by the kernel.

The output of the hardware USB analyzer told me that the host doesn't
even poll for interrupt IN transfers (even the "interrupt in" USB
transfer are polled by the host).

After some debugging in hid-core.c I've found the reason.

In case of a highspeed device, the endpoint interval is re-calculated in
driver/usb/input/hid-core.c:

line 1669:
             /* handle potential highspeed HID correctly */
             interval = endpoint->bInterval;
             if (dev->speed == USB_SPEED_HIGH)
                   interval = 1 << (interval - 1);

Basically this calculation is correct (refer to USB 2.0 spec, 9.6.6).
This new calculated value of "interval" is used as input for
usb_fill_int_urb:

line 1685:

            usb_fill_int_urb(hid->urbin, dev, pipe, hid->inbuf, 0,
                   hid_irq_in, hid, interval);

Unfortunately the same calculation as above is done a second time in 
usb_fill_int_urb in the file include/linux/usb.h:

line 933:
        if (dev->speed == USB_SPEED_HIGH)
                urb->interval = 1 << (interval - 1);
        else
                urb->interval = interval;

This means, that if the endpoint descriptor (of a high speed device)
specifies e.g. bInterval = 7, the urb->interval gets the value:

hid-core.c: interval = 1 << (7-1) = 0x40 = 64
urb->interval = 1 << (interval -1) = 1 << (63) = integer overflow

Because of this the value of urb->interval is sometimes negative and is
rejected in core/urb.c:
line 353:
                /* too small? */
                if (urb->interval <= 0)
                        return -EINVAL;

The conclusion is, that the recalculaton of the interval (which is
necessary for highspeed) should not be made twice, because this is
simply wrong. ;-)

Re-calculation in usb_fill_int_urb makes more sense, because it is the
most general approach. So it would make sense to remove it from
hid-core.c.

Because in hid-core.c the interval variable is only used for calling
usb_fill_int_urb, it is no problem to remove the highspeed
re-calculation in this file.


Signed-off-by: Christian Krause <chkr@plauener.de>


--------------------snip----------------------
--- linux-2.6.13.4/drivers/usb/input/hid-core.c.old	2005-10-15 09:24:22.000000000 +0200
+++ linux-2.6.13.4/drivers/usb/input/hid-core.c	2005-10-15 09:55:50.000000000 +0200
@@ -1667,10 +1667,7 @@ static struct hid_device *usb_hid_config
 		if ((endpoint->bmAttributes & 3) != 3)		/* Not an interrupt endpoint */
 			continue;
 
-		/* handle potential highspeed HID correctly */
 		interval = endpoint->bInterval;
-		if (dev->speed == USB_SPEED_HIGH)
-			interval = 1 << (interval - 1);
 
 		/* Change the polling interval of mice. */
 		if (hid->collection->usage == HID_GD_MOUSE && hid_mousepoll_interval > 0)
--------------------snip----------------------


Best regards,
Christian

[PATCH] Re: bug in handling of highspeed usb HID devices

[Christian Krause]

Hi Greg,

On Thu, 13 Oct 2005 15:48:39 -0700, Greg KH wrote:
> On Wed, Oct 12, 2005 at 09:55:32PM +0200, Christian Krause wrote:
>> Here is a small patch which solves the whole problem:

> The patch is at the wrong level, and has spaces instead of tabs.
> And no "signed-off-by" line :(
> Take a look at Documentation/SubmittingPatches for how to create a patch
> that I can apply and forward on.

Please apologize the wrong format of the patch, here is the next
try. I also include the description why the change is necessary again:

During the development of an USB device I found a bug in the handling of
Highspeed HID devices in the kernel.

What happened?

Highspeed HID devices are correctly recognized and enumerated by the
kernel. But even if usbhid kernel module is loaded, no HID reports are
received by the kernel.

The output of the hardware USB analyzer told me that the host doesn't
even poll for interrupt IN transfers (even the "interrupt in" USB
transfer are polled by the host).

After some debugging in hid-core.c I've found the reason.

In case of a highspeed device, the endpoint interval is re-calculated in
driver/usb/input/hid-core.c:

line 1669:
             /* handle potential highspeed HID correctly */
             interval = endpoint->bInterval;
             if (dev->speed == USB_SPEED_HIGH)
                   interval = 1 << (interval - 1);

Basically this calculation is correct (refer to USB 2.0 spec, 9.6.6).
This new calculated value of "interval" is used as input for
usb_fill_int_urb:

line 1685:

            usb_fill_int_urb(hid->urbin, dev, pipe, hid->inbuf, 0,
                   hid_irq_in, hid, interval);

Unfortunately the same calculation as above is done a second time in 
usb_fill_int_urb in the file include/linux/usb.h:

line 933:
        if (dev->speed == USB_SPEED_HIGH)
                urb->interval = 1 << (interval - 1);
        else
                urb->interval = interval;

This means, that if the endpoint descriptor (of a high speed device)
specifies e.g. bInterval = 7, the urb->interval gets the value:

hid-core.c: interval = 1 << (7-1) = 0x40 = 64
urb->interval = 1 << (interval -1) = 1 << (63) = integer overflow

Because of this the value of urb->interval is sometimes negative and is
rejected in core/urb.c:
line 353:
                /* too small? */
                if (urb->interval <= 0)
                        return -EINVAL;

The conclusion is, that the recalculaton of the interval (which is
necessary for highspeed) should not be made twice, because this is
simply wrong. ;-)

Re-calculation in usb_fill_int_urb makes more sense, because it is the
most general approach. So it would make sense to remove it from
hid-core.c.

Because in hid-core.c the interval variable is only used for calling
usb_fill_int_urb, it is no problem to remove the highspeed
re-calculation in this file.

--------------------------------snip------------------------
--- linux-2.6.13.4/drivers/usb/input/hid-core.c.old	2005-10-12 21:29:29.000000000 +0200
+++ linux-2.6.13.4/drivers/usb/input/hid-core.c	2005-10-12 21:31:02.000000000 +0200
@@ -1667,11 +1667,6 @@ static struct hid_device *usb_hid_config
 		if ((endpoint->bmAttributes & 3) != 3)		/* Not an interrupt endpoint */
 			continue;
 
-		/* handle potential highspeed HID correctly */
-		interval = endpoint->bInterval;
-		if (dev->speed == USB_SPEED_HIGH)
-			interval = 1 << (interval - 1);
-
 		/* Change the polling interval of mice. */
 		if (hid->collection->usage == HID_GD_MOUSE && hid_mousepoll_interval > 0)
 			interval = hid_mousepoll_interval;

--------------------------------snip------------------------
Signed-off-by: Christian Krause <chkr@plauener.de>


I hope all things are ok now and I ask kindly for applying. 

> Also, what device needs this patch?  Is it a device that I can buy
> today?

Yes, just buy Avocent's DSR2030. It enumerates as a highspeed HID device.


Best regards,
Christian

Re: [PATCH] Re: bug in handling of highspeed usb HID devices

[Greg KH]

On Fri, Oct 14, 2005 at 07:57:45PM +0200, Christian Krause wrote:
> Hi Greg,
> 
> On Thu, 13 Oct 2005 15:48:39 -0700, Greg KH wrote:
> > On Wed, Oct 12, 2005 at 09:55:32PM +0200, Christian Krause wrote:
> >> Here is a small patch which solves the whole problem:
> 
> > The patch is at the wrong level, and has spaces instead of tabs.
> > And no "signed-off-by" line :(
> > Take a look at Documentation/SubmittingPatches for how to create a patch
> > that I can apply and forward on.
> 
> Please apologize the wrong format of the patch, here is the next
> try. I also include the description why the change is necessary again:

You forgot a Signed-off-by: in the patch description.  Anything below
the patch is thrown away by our scripts.

Care to try it again?  Third time's a charm :)

thanks,

greg k-h

[PATCH] Re: bug in handling of highspeed usb HID devices

[kernel-stuff]

> Ok, then only one question remains: How do I get this patch applied to
> the official kernel tree?
> 
> Thanks & best regards,
> Christian

I already forwarded your patch to linux-usb-devel@lists.sf.net. 
No one seems to have picked it up till now.

This seems to be -stable material since it's a clear cut bug with bad
consequences. 

Chris Wright - is the below patch acceptable for -stable?

If no one else cares, there is always Andrew Morton and -mm ! :)

Parag



----------------------  Forwarded Message:  ---------------------
From:    Christian Krause <chkr@plauener.de>
To:      linux-kernel@vger.kernel.org
Subject: bug in handling of highspeed usb HID devices
Date:    Wed, 12 Oct 2005 19:56:16 +0000

Hi folks,

During the development of an USB device I found a bug in the handling of
Highspeed HID devices in the kernel.

What happened?

Highspeed HID devices are correctly recognized and enumerated by the
kernel. But even if usbhid kernel module is loaded, no HID reports are
received by the kernel.

The output of the hardware USB analyzer told me that the host doesn't
even poll for interrupt IN transfers (even the "interrupt in" USB
transfer are polled by the host).


After some debugging in hid-core.c I've found the reason.

In case of a highspeed device, the endpoint interval is re-calculated in
driver/usb/input/hid-core.c:

line 1669:
             /* handle potential highspeed HID correctly */
             interval = endpoint->bInterval;
             if (dev->speed == USB_SPEED_HIGH)
                   interval = 1 << (interval - 1);

Basically this calculation is correct (refer to USB 2.0 spec, 9.6.6).
This new calculated value of "interval" is used as input for
usb_fill_int_urb:

line 1685:

            usb_fill_int_urb(hid->urbin, dev, pipe, hid->inbuf, 0,
                   hid_irq_in, hid, interval);

Unfortunately the same calculation as above is done a second time in 
usb_fill_int_urb in the file include/linux/usb.h:

line 933:
        if (dev->speed == USB_SPEED_HIGH)
                urb->interval = 1 << (interval - 1);
        else
                urb->interval = interval;

This means, that if the endpoint descriptor (of a high speed device)
specifies e.g. bInterval = 7, the urb->interval gets the value:

hid-core.c: interval = 1 << (7-1) = 0x40 = 64
urb->interval = 1 << (interval -1) = 1 << (63) = integer overflow

Because of this the value of urb->interval is sometimes negative and is
rejected in core/urb.c:
line 353:
                /* too small? */
                if (urb->interval <= 0)
                        return -EINVAL;


The conclusion is, that the recalculaton of the interval (which is
necessary for highspeed) should not be made twice, because this is
simply wrong. ;-)


Re-calculation in usb_fill_int_urb makes more sense, because it is the
most general approach. So it would make sense to remove it from
hid-core.c.


Because in hid-core.c the interval variable is only used for calling
usb_fill_int_urb, it is no problem to remove the highspeed
re-calculation in this file.

Here is a small patch which solves the whole problem:

--------------------------
--- hid-core.c.old      2005-10-12 21:29:29.000000000 +0200
+++ hid-core.c  2005-10-12 21:31:02.000000000 +0200
@@ -1667,11 +1667,6 @@
                if ((endpoint->bmAttributes & 3) != 3)          /* Not an 
interrupt endpoint */
                        continue;
 
-               /* handle potential highspeed HID correctly */
-               interval = endpoint->bInterval;
-               if (dev->speed == USB_SPEED_HIGH)
-                       interval = 1 << (interval - 1);
-
                /* Change the polling interval of mice. */
                if (hid->collection->usage == HID_GD_MOUSE && 
hid_mousepoll_interval > 0)
                        interval = hid_mousepoll_interval;

----------------------------

Please review my investigation and if you come to the same conclusion
please apply this patch in the next kernel versions. If not, please tell
me why. ;-)


Best regards,
Christian

Re: [PATCH] Re: bug in handling of highspeed usb HID devices

[Chris Wright]

* kernel-stuff@comcast.net (kernel-stuff@comcast.net) wrote:
> This seems to be -stable material since it's a clear cut bug with bad
> consequences. 
> 
> Chris Wright - is the below patch acceptable for -stable?

This all looks fine.  But two things, please send -stable patches to
stable@kernel.org, and I've not seen an ACK from any usb developers.

thanks,
-chris