From patchwork Mon Mar 29 13:41:18 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefan Bader X-Patchwork-Id: 48843 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from chlorine.canonical.com (chlorine.canonical.com [91.189.94.204]) by ozlabs.org (Postfix) with ESMTP id BFF63B7CEC for ; Tue, 30 Mar 2010 00:41:32 +1100 (EST) Received: from localhost ([127.0.0.1] helo=chlorine.canonical.com) by chlorine.canonical.com with esmtp (Exim 4.69) (envelope-from ) id 1NwFDX-0003Xm-PX; Mon, 29 Mar 2010 14:41:23 +0100 Received: from adelie.canonical.com ([91.189.90.139]) by chlorine.canonical.com with esmtp (Exim 4.69) (envelope-from ) id 1NwFDU-0003W1-L0 for kernel-team@lists.ubuntu.com; Mon, 29 Mar 2010 14:41:20 +0100 Received: from hutte.canonical.com ([91.189.90.181]) by adelie.canonical.com with esmtp (Exim 4.69 #1 (Debian)) id 1NwFDT-0008K0-VT; Mon, 29 Mar 2010 14:41:20 +0100 Received: from p5b2e5e51.dip.t-dialin.net ([91.46.94.81] helo=[192.168.2.121]) by hutte.canonical.com with esmtpsa (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.69) (envelope-from ) id 1NwFDT-0000kx-OY; Mon, 29 Mar 2010 14:41:19 +0100 Message-ID: <4BB0ADFE.6010403@canonical.com> Date: Mon, 29 Mar 2010 15:41:18 +0200 From: Stefan Bader User-Agent: Thunderbird 2.0.0.24 (X11/20100317) MIME-Version: 1.0 To: Chase Douglas Subject: Re: USBFS Bugfix References: <40ec3ea41003240934i7226e6dp96e8d3564186d188@mail.gmail.com> <4BAA4059.4040000@canonical.com> <40ec3ea41003240944r24ff28d5w9e8e371c6df85c18@mail.gmail.com> In-Reply-To: <40ec3ea41003240944r24ff28d5w9e8e371c6df85c18@mail.gmail.com> X-Enigmail-Version: 0.95.7 Cc: kernel-team@lists.ubuntu.com X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.9 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kernel-team-bounces@lists.ubuntu.com Errors-To: kernel-team-bounces@lists.ubuntu.com Chase Douglas wrote: > On Wed, Mar 24, 2010 at 12:39 PM, Stefan Bader > wrote: >> Chase Douglas wrote: >>> Hi Markus, >>> >>> On Wed, Mar 24, 2010 at 12:21 PM, Markus Rechberger >>> 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 >>>> CC: stable >>>> >>>> --- >>>> 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 >> 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 As this patch is upstream and has been cc'ed to stable too, I would ACK to cherry-pick the upstream patch (attached for simplicity) Stefan Acked-by: Andy Whitcroft From 7152b592593b9d48b33f8997b1dfd6df9143f7ec Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Sat, 6 Mar 2010 15:04:03 -0500 Subject: [PATCH] USB: fix usbfs regression This patch (as1352) 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. Reported-by: Markus Rechberger Signed-off-by: Alan Stern CC: stable Signed-off-by: Greg Kroah-Hartman --- drivers/usb/core/devio.c | 17 ++++++++++++++--- 1 files changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index e909ff7..3466fdc 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1207,6 +1207,13 @@ static int proc_do_submiturb(struct dev_state *ps, struct usbdevfs_urb *uurb, 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) | @@ -1345,10 +1352,14 @@ static int processcompl(struct async *as, void __user * __user *arg) 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)) -- 1.6.3.3