Re: usb/storage/uas: slab-out-of-bounds in uas_probe

[Andrey Konovalov]

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)
>