Patchwork USBFS Bugfix

login
register
mail settings
Submitter Markus Rechberger
Date March 24, 2010, 4:21 p.m.
Message ID <d9def9db1003240921g5c023ccdp7fb94eb1f872b1a9@mail.gmail.com>
Download mbox | patch
Permalink /patch/48437/
State Superseded
Headers show

Comments

Markus Rechberger - March 24, 2010, 4:21 p.m.
Hi,

on IRC I was recommended to submit this information to the mailinglist

https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527

Here's a better solution.  In theory we could copy just the individual
packets from within the transfer buffer, but that would probably take
longer than simply copying the whole buffer.

(This was a little hasty; I haven't even compile-tested the patch.
Some small fixes may be needed.)

Alan Stern


-----------------------------------------------------------------------
This patch fixes a bug in the way isochronous input data is returned
to userspace for usbfs transfers.  The entire buffer must be copied,
not just the first actual_length bytes, because the individual packets
will be discontiguous if any of them are short.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: stable <stable@kernel.org>

---

It would be nice if this patch could go into the ubuntu lucid kernel
as soon as possible.

Thanks,
Markus
Chase Douglas - March 24, 2010, 4:34 p.m.
Hi Markus,

On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger
<mrechberger@gmail.com> wrote:
> Hi,
>
> on IRC I was recommended to submit this information to the mailinglist
>
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527

The bug itself doesn't list much information. It basically says "usbfs
is broken." We need some proof of what is going wrong, preferably
something that can be tested when a fix is available.

> Here's a better solution.  In theory we could copy just the individual
> packets from within the transfer buffer, but that would probably take
> longer than simply copying the whole buffer.
>
> (This was a little hasty; I haven't even compile-tested the patch.
> Some small fixes may be needed.)

The general procedure for fixes to reach Ubuntu is to have them sent
upstream and be included in the mainline linux, and then to have the
fix read the -stable tree. We then pull patches from the -stable tree
for inclusion in Ubuntu.

If you can show that a specific patch has been found to fix an issue
in Ubuntu and it has been accepted upstream by the maintainers of the
kernel subsystem this impacts, we may be able to accept the patch as a
pre-stable fix, in anticipation that it will be accepted into the
-stable tree in the future.

What is the origin of this patch? Is it from upstream somewhere? A
commit hash from linux-2.6 or some other tree would be very helpful.

> Alan Stern
>
>
> -----------------------------------------------------------------------
> This patch fixes a bug in the way isochronous input data is returned
> to userspace for usbfs transfers.  The entire buffer must be copied,
> not just the first actual_length bytes, because the individual packets
> will be discontiguous if any of them are short.
>
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: stable <stable@kernel.org>
>
> ---
> Index: usb-2.6/drivers/usb/core/devio.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/core/devio.c
> +++ usb-2.6/drivers/usb/core/devio.c
> @@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
>                        free_async(as);
>                        return -ENOMEM;
>                }
> +               /* Isochronous input data may end up being discontiguous
> +                * if some of the packets are short.  Clear the buffer so
> +                * that the gaps don't leak kernel data to userspace.
> +                */
> +               if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
> +                       memset(as->urb->transfer_buffer, 0,
> +                                       uurb->buffer_length);
>        }
>        as->urb->dev = ps->dev;
>        as->urb->pipe = (uurb->type << 30) |
> @@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
>        void __user *addr = as->userurb;
>        unsigned int i;
>
> -       if (as->userbuffer && urb->actual_length)
> -               if (copy_to_user(as->userbuffer, urb->transfer_buffer,
> -                                urb->actual_length))
> +       if (as->userbuffer && urb->actual_length) {
> +               if (urb->number_of_packets > 0)         /* Isochronous */
> +                       i = urb->transfer_buffer_length;
> +               else                                    /* Non-Isoc */
> +                       i = urb->actual_length;
> +               if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
>                        goto err_out;
> +       }
>        if (put_user(as->status, &userurb->status))
>                goto err_out;
>        if (put_user(urb->actual_length, &userurb->actual_length))
>
> It would be nice if this patch could go into the ubuntu lucid kernel
> as soon as possible.
>
> Thanks,
> Markus

-- Chase
Markus Rechberger - March 24, 2010, 4:39 p.m.
On Wed, Mar 24, 2010 at 5:34 PM, Chase Douglas
<chase.douglas@canonical.com> wrote:
> Hi Markus,
>
> On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger
> <mrechberger@gmail.com> wrote:
>> Hi,
>>
>> on IRC I was recommended to submit this information to the mailinglist
>>
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527
>
> The bug itself doesn't list much information. It basically says "usbfs
> is broken." We need some proof of what is going wrong, preferably
> something that can be tested when a fix is available.
>
>> Here's a better solution.  In theory we could copy just the individual
>> packets from within the transfer buffer, but that would probably take
>> longer than simply copying the whole buffer.
>>
>> (This was a little hasty; I haven't even compile-tested the patch.
>> Some small fixes may be needed.)
>
> The general procedure for fixes to reach Ubuntu is to have them sent
> upstream and be included in the mainline linux, and then to have the
> fix read the -stable tree. We then pull patches from the -stable tree
> for inclusion in Ubuntu.
>
> If you can show that a specific patch has been found to fix an issue
> in Ubuntu and it has been accepted upstream by the maintainers of the
> kernel subsystem this impacts, we may be able to accept the patch as a
> pre-stable fix, in anticipation that it will be accepted into the
> -stable tree in the future.
>
> What is the origin of this patch? Is it from upstream somewhere? A
> commit hash from linux-2.6 or some other tree would be very helpful.

Hi,

sorry I'm a little bit busy
the patch is already upstream but not in ubuntu right now I tested
Ubuntu Lucid yesterday
and it's affected by that bug

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=7152b592593b9d48b33f8997b1dfd6df9143f7ec

Markus
>
>> Alan Stern
>>
>>
>> -----------------------------------------------------------------------
>> This patch fixes a bug in the way isochronous input data is returned
>> to userspace for usbfs transfers.  The entire buffer must be copied,
>> not just the first actual_length bytes, because the individual packets
>> will be discontiguous if any of them are short.
>>
>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>> CC: stable <stable@kernel.org>
>>
>> ---
>> Index: usb-2.6/drivers/usb/core/devio.c
>> ===================================================================
>> --- usb-2.6.orig/drivers/usb/core/devio.c
>> +++ usb-2.6/drivers/usb/core/devio.c
>> @@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
>>                        free_async(as);
>>                        return -ENOMEM;
>>                }
>> +               /* Isochronous input data may end up being discontiguous
>> +                * if some of the packets are short.  Clear the buffer so
>> +                * that the gaps don't leak kernel data to userspace.
>> +                */
>> +               if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
>> +                       memset(as->urb->transfer_buffer, 0,
>> +                                       uurb->buffer_length);
>>        }
>>        as->urb->dev = ps->dev;
>>        as->urb->pipe = (uurb->type << 30) |
>> @@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
>>        void __user *addr = as->userurb;
>>        unsigned int i;
>>
>> -       if (as->userbuffer && urb->actual_length)
>> -               if (copy_to_user(as->userbuffer, urb->transfer_buffer,
>> -                                urb->actual_length))
>> +       if (as->userbuffer && urb->actual_length) {
>> +               if (urb->number_of_packets > 0)         /* Isochronous */
>> +                       i = urb->transfer_buffer_length;
>> +               else                                    /* Non-Isoc */
>> +                       i = urb->actual_length;
>> +               if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
>>                        goto err_out;
>> +       }
>>        if (put_user(as->status, &userurb->status))
>>                goto err_out;
>>        if (put_user(urb->actual_length, &userurb->actual_length))
>>
>> It would be nice if this patch could go into the ubuntu lucid kernel
>> as soon as possible.
>>
>> Thanks,
>> Markus
>
> -- Chase
>
Stefan Bader - March 24, 2010, 4:39 p.m.
Chase Douglas wrote:
> Hi Markus,
> 
> On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger
> <mrechberger@gmail.com> wrote:
>> Hi,
>>
>> on IRC I was recommended to submit this information to the mailinglist
>>
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527
> 
> The bug itself doesn't list much information. It basically says "usbfs
> is broken." We need some proof of what is going wrong, preferably
> something that can be tested when a fix is available.
> 
>> Here's a better solution.  In theory we could copy just the individual
>> packets from within the transfer buffer, but that would probably take
>> longer than simply copying the whole buffer.
>>
>> (This was a little hasty; I haven't even compile-tested the patch.
>> Some small fixes may be needed.)
> 
> The general procedure for fixes to reach Ubuntu is to have them sent
> upstream and be included in the mainline linux, and then to have the
> fix read the -stable tree. We then pull patches from the -stable tree
> for inclusion in Ubuntu.
> 
> If you can show that a specific patch has been found to fix an issue
> in Ubuntu and it has been accepted upstream by the maintainers of the
> kernel subsystem this impacts, we may be able to accept the patch as a
> pre-stable fix, in anticipation that it will be accepted into the
> -stable tree in the future.
> 
> What is the origin of this patch? Is it from upstream somewhere? A
> commit hash from linux-2.6 or some other tree would be very helpful.
> 
>> Alan Stern
>>
>>
>> -----------------------------------------------------------------------
>> This patch fixes a bug in the way isochronous input data is returned
>> to userspace for usbfs transfers.  The entire buffer must be copied,
>> not just the first actual_length bytes, because the individual packets
>> will be discontiguous if any of them are short.
>>
>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>> CC: stable <stable@kernel.org>
>>
>> ---
>> Index: usb-2.6/drivers/usb/core/devio.c
>> ===================================================================
>> --- usb-2.6.orig/drivers/usb/core/devio.c
>> +++ usb-2.6/drivers/usb/core/devio.c
>> @@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
>>                        free_async(as);
>>                        return -ENOMEM;
>>                }
>> +               /* Isochronous input data may end up being discontiguous
>> +                * if some of the packets are short.  Clear the buffer so
>> +                * that the gaps don't leak kernel data to userspace.
>> +                */
>> +               if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
>> +                       memset(as->urb->transfer_buffer, 0,
>> +                                       uurb->buffer_length);
>>        }
>>        as->urb->dev = ps->dev;
>>        as->urb->pipe = (uurb->type << 30) |
>> @@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
>>        void __user *addr = as->userurb;
>>        unsigned int i;
>>
>> -       if (as->userbuffer && urb->actual_length)
>> -               if (copy_to_user(as->userbuffer, urb->transfer_buffer,
>> -                                urb->actual_length))
>> +       if (as->userbuffer && urb->actual_length) {
>> +               if (urb->number_of_packets > 0)         /* Isochronous */
>> +                       i = urb->transfer_buffer_length;
>> +               else                                    /* Non-Isoc */
>> +                       i = urb->actual_length;
>> +               if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
>>                        goto err_out;
>> +       }
>>        if (put_user(as->status, &userurb->status))
>>                goto err_out;
>>        if (put_user(urb->actual_length, &userurb->actual_length))
>>
>> It would be nice if this patch could go into the ubuntu lucid kernel
>> as soon as possible.
>>
>> Thanks,
>> Markus
> 
> -- Chase
> 
Well it seems to come from Alan Stern and be cc'ed to stable. So it should/would
come from there. But I have not seen it in the queue, yet. But it is in upstream:

commit 7152b592593b9d48b33f8997b1dfd6df9143f7ec
Author: Alan Stern <stern@rowland.harvard.edu>
Date:   Sat Mar 6 15:04:03 2010 -0500

    USB: fix usbfs regression
Chase Douglas - March 24, 2010, 4:44 p.m.
On Wed, Mar 24, 2010 at 12:39 PM, Stefan Bader
<stefan.bader@canonical.com> wrote:
> Chase Douglas wrote:
>> Hi Markus,
>>
>> On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger
>> <mrechberger@gmail.com> wrote:
>>> Hi,
>>>
>>> on IRC I was recommended to submit this information to the mailinglist
>>>
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/544527
>>
>> The bug itself doesn't list much information. It basically says "usbfs
>> is broken." We need some proof of what is going wrong, preferably
>> something that can be tested when a fix is available.
>>
>>> Here's a better solution.  In theory we could copy just the individual
>>> packets from within the transfer buffer, but that would probably take
>>> longer than simply copying the whole buffer.
>>>
>>> (This was a little hasty; I haven't even compile-tested the patch.
>>> Some small fixes may be needed.)
>>
>> The general procedure for fixes to reach Ubuntu is to have them sent
>> upstream and be included in the mainline linux, and then to have the
>> fix read the -stable tree. We then pull patches from the -stable tree
>> for inclusion in Ubuntu.
>>
>> If you can show that a specific patch has been found to fix an issue
>> in Ubuntu and it has been accepted upstream by the maintainers of the
>> kernel subsystem this impacts, we may be able to accept the patch as a
>> pre-stable fix, in anticipation that it will be accepted into the
>> -stable tree in the future.
>>
>> What is the origin of this patch? Is it from upstream somewhere? A
>> commit hash from linux-2.6 or some other tree would be very helpful.
>>
>>> Alan Stern
>>>
>>>
>>> -----------------------------------------------------------------------
>>> This patch fixes a bug in the way isochronous input data is returned
>>> to userspace for usbfs transfers.  The entire buffer must be copied,
>>> not just the first actual_length bytes, because the individual packets
>>> will be discontiguous if any of them are short.
>>>
>>> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>>> CC: stable <stable@kernel.org>
>>>
>>> ---
>>> Index: usb-2.6/drivers/usb/core/devio.c
>>> ===================================================================
>>> --- usb-2.6.orig/drivers/usb/core/devio.c
>>> +++ usb-2.6/drivers/usb/core/devio.c
>>> @@ -1176,6 +1176,13 @@ static int proc_do_submiturb(struct dev_
>>>                        free_async(as);
>>>                        return -ENOMEM;
>>>                }
>>> +               /* Isochronous input data may end up being discontiguous
>>> +                * if some of the packets are short.  Clear the buffer so
>>> +                * that the gaps don't leak kernel data to userspace.
>>> +                */
>>> +               if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
>>> +                       memset(as->urb->transfer_buffer, 0,
>>> +                                       uurb->buffer_length);
>>>        }
>>>        as->urb->dev = ps->dev;
>>>        as->urb->pipe = (uurb->type << 30) |
>>> @@ -1312,10 +1319,14 @@ static int processcompl(struct async *as
>>>        void __user *addr = as->userurb;
>>>        unsigned int i;
>>>
>>> -       if (as->userbuffer && urb->actual_length)
>>> -               if (copy_to_user(as->userbuffer, urb->transfer_buffer,
>>> -                                urb->actual_length))
>>> +       if (as->userbuffer && urb->actual_length) {
>>> +               if (urb->number_of_packets > 0)         /* Isochronous */
>>> +                       i = urb->transfer_buffer_length;
>>> +               else                                    /* Non-Isoc */
>>> +                       i = urb->actual_length;
>>> +               if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
>>>                        goto err_out;
>>> +       }
>>>        if (put_user(as->status, &userurb->status))
>>>                goto err_out;
>>>        if (put_user(urb->actual_length, &userurb->actual_length))
>>>
>>> It would be nice if this patch could go into the ubuntu lucid kernel
>>> as soon as possible.
>>>
>>> Thanks,
>>> Markus
>>
>> -- Chase
>>
> Well it seems to come from Alan Stern and be cc'ed to stable. So it should/would
> come from there. But I have not seen it in the queue, yet. But it is in upstream:
>
> commit 7152b592593b9d48b33f8997b1dfd6df9143f7ec
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date:   Sat Mar 6 15:04:03 2010 -0500
>
>    USB: fix usbfs regression

Ahh, I didn't realize Alan is the maintainer of the code. I'll leave
it up to you, Stefan, to determine how to handle this.

-- Chase
Stefan Bader - March 29, 2010, 2:29 p.m.
One addition:
It might be that we are talking about USB_DEVICEFS. And that is not active anymore.

Stefan
Stefan Bader - March 29, 2010, 2:49 p.m.
Stefan Bader wrote:
> One addition:
> It might be that we are talking about USB_DEVICEFS. And that is not active anymore.
> 
> Stefan
> 
Ok, upon feedback from Markus, he does not need the usb filesystem being mounted.

Patch

Index: usb-2.6/drivers/usb/core/devio.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/devio.c
+++ usb-2.6/drivers/usb/core/devio.c
@@ -1176,6 +1176,13 @@  static int proc_do_submiturb(struct dev_
 			free_async(as);
 			return -ENOMEM;
 		}
+		/* Isochronous input data may end up being discontiguous
+		 * if some of the packets are short.  Clear the buffer so
+		 * that the gaps don't leak kernel data to userspace.
+		 */
+		if (is_in && uurb->type == USBDEVFS_URB_TYPE_ISO)
+			memset(as->urb->transfer_buffer, 0,
+					uurb->buffer_length);
 	}
 	as->urb->dev = ps->dev;
 	as->urb->pipe = (uurb->type << 30) |
@@ -1312,10 +1319,14 @@  static int processcompl(struct async *as
 	void __user *addr = as->userurb;
 	unsigned int i;

-	if (as->userbuffer && urb->actual_length)
-		if (copy_to_user(as->userbuffer, urb->transfer_buffer,
-				 urb->actual_length))
+	if (as->userbuffer && urb->actual_length) {
+		if (urb->number_of_packets > 0)		/* Isochronous */
+			i = urb->transfer_buffer_length;
+		else					/* Non-Isoc */
+			i = urb->actual_length;
+		if (copy_to_user(as->userbuffer, urb->transfer_buffer, i))
 			goto err_out;
+	}
 	if (put_user(as->status, &userurb->status))
 		goto err_out;
 	if (put_user(urb->actual_length, &userurb->actual_length))