[PATCH] usbnet.c, buf.overrun crash-bugfix, Kernel 2.6.12-rc1

[PATCH] usbnet.c, buf.overrun crash-bugfix, Kernel 2.6.12-rc1

[Jakemuksen spammiosote]

Atleast versions 2.6.5 - 2.6.12-rc1 crash if an USB device using usbnet 
sends oversized packet. Such packets occur most likely with broken
device. Here's a patch that throws away such packet, to keep the machine
from crashing. Hopefully this doesn't leave memory unreleased. If it does, 
it's still better than crashing as such oversized packets are really rare.

Signed-off-by: Jarkko Hakala <jhroska@byterapers.com>

diff -Nur linux-2.6.12-rc1-orig/drivers/usb/net/usbnet.c 
linux-2.6.12-rc1/drivers/usb/net/usbnet.c
--- linux-2.6.12-rc1-orig/drivers/usb/net/usbnet.c      2005-03-18 
03:34:13.000000000 +0200
+++ linux-2.6.12-rc1/drivers/usb/net/usbnet.c   2005-03-24 
16:46:08.000000000 +0200
@@ -2795,9 +2795,20 @@
         struct usbnet           *dev = entry->dev;
         int                     urb_status = urb->status;

-       skb_put (skb, urb->actual_length);
-       entry->state = rx_done;
-       entry->urb = NULL;
+       if (unlikely((skb->tail + urb->actual_length) > skb->end)) {
+               entry->state = rx_cleanup;
+               dev->stats.rx_errors++;
+               dev->stats.rx_length_errors++;
+               entry->urb = NULL;
+               printk(KERN_ERR
+                      "USB RX packet too long, discarded. "
+                      "Your slave device most likely is broken\n");
+               /* lets hope upper level protocols will recover */
+       } else {
+               skb_put(skb, urb->actual_length);
+               entry->state = rx_done;
+               entry->urb = NULL;
+       }

         switch (urb_status) {
             // success

Re: [PATCH] usbnet.c, buf.overrun crash-bugfix, Kernel 2.6.12-rc1

[David Brownell]

On Thursday 24 March 2005 8:05 am, Jakemuksen spammiosote wrote:
> Atleast versions 2.6.5 - 2.6.12-rc1 crash if an USB device using usbnet 
> sends oversized packet. Such packets occur most likely with broken
> device. 

Care to mention what device(s) you saw this with?   And what HCD?


> -       skb_put (skb, urb->actual_length);
> -       entry->state = rx_done;
> -       entry->urb = NULL;
> +       if (unlikely((skb->tail + urb->actual_length) > skb->end)) {

This logic looks wrong.  If that ever happens, surely the problem is
that the rx_submit() code submitted an urb with transfer_size that
mismatched the SKB.  The host controller isn't allowed to overrun the
end of the buffer it's passed.  And if it's tempted to do so, it's
supposed to fill up to the end (skb->end in this case...) and then
report urb->status of -EOVERFLOW.

If you insist on changing this bit of logic, then the best way to
ignore the packet is just to force urb->status to -EOVERFLOW


> +               entry->state = rx_cleanup;
> +               dev->stats.rx_errors++;
> +               dev->stats.rx_length_errors++;
> +               entry->urb = NULL;
> +               printk(KERN_ERR
> +                      "USB RX packet too long, discarded. "
> +                      "Your slave device most likely is broken\n");
> +               /* lets hope upper level protocols will recover */
> +       } else {
> +               skb_put(skb, urb->actual_length);
> +               entry->state = rx_done;
> +               entry->urb = NULL;
> +       }
> 
>          switch (urb_status) {
>              // success
> 
> 

Re: [PATCH] usbnet.c, buf.overrun crash-bugfix, Kernel 2.6.12-rc1

[Jakemuksen spammiosote]

On Thu, 24 Mar 2005, David Brownell wrote:
> On Thursday 24 March 2005 8:05 am, Jakemuksen spammiosote wrote:
>> Atleast versions 2.6.5 - 2.6.12-rc1 crash if an USB device using usbnet
>> sends oversized packet. Such packets occur most likely with broken
> Care to mention what device(s) you saw this with?   And what HCD?

I can't tell about the device(NDA), and don't remember the HCD and I can 
check it only after holidays.

>> +       if (unlikely((skb->tail + urb->actual_length) > skb->end)) {
> This logic looks wrong.  If that ever happens, surely the problem is
> that the rx_submit() code submitted an urb with transfer_size that
> mismatched the SKB.  The host controller isn't allowed to overrun the

Sounds reasonable. So, I'll go thru the HCD code instead if the 
responsibility is there. Am i the first one to run into such crash 
situation? If so, perhaps it's not ever worthy to fix in mainstream 
kernel, as the device causes the crash under very specific - 
'abusing' one might say, situation only.

Re: [PATCH] usbnet.c, buf.overrun crash-bugfix, Kernel 2.6.12-rc1

[David Brownell]

On Thursday 24 March 2005 10:13 am, Jakemuksen spammiosote wrote:
> On Thu, 24 Mar 2005, David Brownell wrote:
> > On Thursday 24 March 2005 8:05 am, Jakemuksen spammiosote wrote:
> 
> >> +       if (unlikely((skb->tail + urb->actual_length) > skb->end)) {
> > 
> > This logic looks wrong.  If that ever happens, surely the problem is 
> > that the rx_submit() code submitted an urb with transfer_size that
> > mismatched the SKB.  The host controller isn't allowed to overrun the
> 
> Sounds reasonable. So, I'll go thru the HCD code

Better yet, start with the code supporting that device you're
under NDA for.


> instead if the  
> responsibility is there. Am i the first one to run into such crash 
> situation? If so, perhaps it's not ever worthy to fix in mainstream 
> kernel, as the device causes the crash under very specific - 
> 'abusing' one might say, situation only.

You're the first one to report such a problem.

- Dave