On Thu, 21 Sep 2017, Andrey Konovalov wrote:
> On Thu, Sep 21, 2017 at 6:10 PM, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Thu, Sep 21, 2017 at 05:39:05PM +0200, Andrey Konovalov wrote:
> >> Hi!
> >>
> >> I've got the following report while fuzzing the kernel with syzkaller.
> >>
> >> On commit ebb2c2437d8008d46796902ff390653822af6cc4 (Sep 18).
> >>
> >> The issue occurs when we iterate over interface altsettings, but I
> >> don't see the driver doing anything wrong. I might be missing
> >> something, or this might be an issue in USB core altsettings parsing.
> >
> >
> > Any chance you happen to have the descriptors that you were feeding into
> > the kernel at this crash? That might help in figuring out what "went
> > wrong".
>
> Here's the data that I feed into dummy_udc:
>
> 00 00 00 00 09 02 12 00 01 34 05 80 07 09 04 6e
> 09 00 08 06 62 00 12 01 05 00 cb f7 71 83 04 00
> 05 00 ab f6 07 81 08 01
>
> Also attaching a C reproducer (requires dummy_hcd and gadgetfs) and my .config.
Why do your reproducers use an mmap'ed array for their data instead of
a plain old statically allocated array?
Anyway, this turns out to be a genuine (and subtle!) bug in the uas
driver. The uas_find_uas_alt_setting() routine in uas-detect.h returns
a bAlternateSetting value, but then the uas_use_uas_driver() routine
uses this value as an index to the altsetting array -- which it isn't.
Normally this doesn't cause any problems because the the entries in the
array have bAlternateSetting values 0, 1, etc., so the value is equal
to the index. But in your fuzzed case, that wasn't true.
The patch below fixes this bug, by returning a pointer to the
alt-setting entry instead of either the value or the index. Pointers
are less subject to misinterpretation.
Alan Stern
Index: usb-4.x/drivers/usb/storage/uas-detect.h
===================================================================
--- usb-4.x.orig/drivers/usb/storage/uas-detect.h
+++ usb-4.x/drivers/usb/storage/uas-detect.h
@@ -9,7 +9,8 @@ static int uas_is_interface(struct usb_h
intf->desc.bInterfaceProtocol == USB_PR_UAS);
}
-static int uas_find_uas_alt_setting(struct usb_interface *intf)
+static struct usb_host_interface *uas_find_uas_alt_setting(
+ struct usb_interface *intf)
{
int i;
@@ -17,10 +18,10 @@ static int uas_find_uas_alt_setting(stru
struct usb_host_interface *alt = &intf->altsetting[i];
if (uas_is_interface(alt))
- return alt->desc.bAlternateSetting;
+ return alt;
}
- return -ENODEV;
+ return NULL;
}
static int uas_find_endpoints(struct usb_host_interface *alt,
@@ -58,14 +59,14 @@ static int uas_use_uas_driver(struct usb
struct usb_device *udev = interface_to_usbdev(intf);
struct usb_hcd *hcd = bus_to_hcd(udev->bus);
unsigned long flags = id->driver_info;
- int r, alt;
-
+ struct usb_host_interface *alt;
+ int r;
alt = uas_find_uas_alt_setting(intf);
- if (alt < 0)
+ if (!alt)
return 0;
- r = uas_find_endpoints(&intf->altsetting[alt], eps);
+ r = uas_find_endpoints(alt, eps);
if (r < 0)
return 0;
Index: usb-4.x/drivers/usb/storage/uas.c
===================================================================
--- usb-4.x.orig/drivers/usb/storage/uas.c
+++ usb-4.x/drivers/usb/storage/uas.c
@@ -873,14 +873,14 @@ MODULE_DEVICE_TABLE(usb, uas_usb_ids);
static int uas_switch_interface(struct usb_device *udev,
struct usb_interface *intf)
{
- int alt;
+ struct usb_host_interface *alt;
alt = uas_find_uas_alt_setting(intf);
- if (alt < 0)
- return alt;
+ if (!alt)
+ return -ENODEV;
- return usb_set_interface(udev,
- intf->altsetting[0].desc.bInterfaceNumber, alt);
+ return usb_set_interface(udev, alt->desc.bInterfaceNumber,
+ alt->desc.bAlternateSetting);
}
static int uas_configure_endpoints(struct uas_dev_info *devinfo)
