| Message ID | 1433164689-8016-1-git-send-email-adam.lee@canonical.com |
|---|---|
| State | New |
| Headers | show |
Looks to be a clean cherry-pick that fixes patches we already have in our trees. The patch is being included in stable. Do you have a way of testing the fix? --chris On 06/01/2015 08:18 AM, Adam Lee wrote: > From: Quentin Casasnovas <quentin.casasnovas@oracle.com> > > BugLink: https://bugs.launchpad.net/1460657 > > Phil and I found out a problem with commit: > > 7e860a6e7aa6 ("cdc-acm: add sanity checks") > > It added some sanity checks to ignore potential garbage in CDC headers but > also introduced a potential infinite loop. This can happen at the first > loop iteration (elength = 0 in that case) if the description isn't a > DT_CS_INTERFACE or later if 'buffer[0]' is zero. > > It should also be noted that the wrong length was being added to 'buffer' > in case 'buffer[1]' was not a DT_CS_INTERFACE descriptor, since elength was > assigned after that check in the loop. > > A specially crafted USB device could be used to trigger this infinite loop. > > Fixes: 7e860a6e7aa6 ("cdc-acm: add sanity checks") > Signed-off-by: Phil Turnbull <phil.turnbull@oracle.com> > Signed-off-by: Quentin Casasnovas <quentin.casasnovas@oracle.com> > CC: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > CC: Oliver Neukum <oneukum@suse.de> > CC: Adam Lee <adam8157@gmail.com> > CC: <stable@vger.kernel.org> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > (cherry picked from commit 0d3bba0287d4e284c3ec7d3397e81eec920d5e7e) > Signed-off-by: Adam Lee <adam.lee@canonical.com> > --- > drivers/usb/class/cdc-acm.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c > index 82dea40..46321cd 100644 > --- a/drivers/usb/class/cdc-acm.c > +++ b/drivers/usb/class/cdc-acm.c > @@ -985,11 +985,16 @@ static int acm_probe(struct usb_interface *intf, > } > > while (buflen > 0) { > + elength = buffer[0]; > + if (!elength) { > + dev_err(&intf->dev, "skipping garbage byte\n"); > + elength = 1; > + goto next_desc; > + } > if (buffer[1] != USB_DT_CS_INTERFACE) { > dev_err(&intf->dev, "skipping garbage\n"); > goto next_desc; > } > - elength = buffer[0]; > > switch (buffer[2]) { > case USB_CDC_UNION_TYPE: /* we've found it */ >
On Mon, Jun 01, 2015 at 08:28:26AM -0500, Chris J Arges wrote: > Looks to be a clean cherry-pick that fixes patches we already have in > our trees. The patch is being included in stable. > > Do you have a way of testing the fix? > > --chris No, is it an issue? Since it's already in stable trees and kind of regression.
On 06/01/2015 08:33 AM, Adam Lee wrote: > On Mon, Jun 01, 2015 at 08:28:26AM -0500, Chris J Arges wrote: >> Looks to be a clean cherry-pick that fixes patches we already have in >> our trees. The patch is being included in stable. >> >> Do you have a way of testing the fix? >> >> --chris > > No, is it an issue? Since it's already in stable trees and kind of > regression. > Generally an SRU needs a way of verifying that the -proposed build with this patch does fix the issue. If the fix can't be directly verified, then some testing of that subsystem would help. In this case it seems like this is a straightforward fix (which is why I acked it). One other thing for whomever applies this, the buglink is incorrectly formatted. --chris
Applied to Trusty, Utopic and Vivid master-next branches.
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 82dea40..46321cd 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -985,11 +985,16 @@ static int acm_probe(struct usb_interface *intf, } while (buflen > 0) { + elength = buffer[0]; + if (!elength) { + dev_err(&intf->dev, "skipping garbage byte\n"); + elength = 1; + goto next_desc; + } if (buffer[1] != USB_DT_CS_INTERFACE) { dev_err(&intf->dev, "skipping garbage\n"); goto next_desc; } - elength = buffer[0]; switch (buffer[2]) { case USB_CDC_UNION_TYPE: /* we've found it */