On Thu, Sep 21, 2017 at 9:04 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> 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?
That's just the way syzkaller generates reproducers, it uses mmap() to
allocate memory to feed into syscalls.
>
> 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.
The patch helps, thanks!
Tested-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
>
> 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)
>
