Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1737017 > unrolled thread
| Started by | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| First post | 2017-09-21 21:10 +0200 |
| Last post | 2017-09-22 13:40 +0200 |
| Articles | 5 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: usb/storage/uas: slab-out-of-bounds in uas_probe Alan Stern <stern@rowland.harvard.edu> - 2017-09-21 21:10 +0200
Re: usb/storage/uas: slab-out-of-bounds in uas_probe Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-22 10:00 +0200
Re: usb/storage/uas: slab-out-of-bounds in uas_probe Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-09-22 10:10 +0200
Re: usb/storage/uas: slab-out-of-bounds in uas_probe Alan Stern <stern@rowland.harvard.edu> - 2017-09-22 17:00 +0200
Re: usb/storage/uas: slab-out-of-bounds in uas_probe Andrey Konovalov <andreyknvl@google.com> - 2017-09-22 13:40 +0200
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2017-09-21 21:10 +0200 |
| Subject | Re: usb/storage/uas: slab-out-of-bounds in uas_probe |
| Message-ID | <usf1f-4Ud-17@gated-at.bofh.it> |
On Thu, 21 Sep 2017, Andrey Konovalov wrote:
> On Thu, Sep 21, 2017 at 6:10 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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)
[toc] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2017-09-22 10:00 +0200 |
| Message-ID | <usr2q-3rE-19@gated-at.bofh.it> |
| In reply to | #1737017 |
On Thu, Sep 21, 2017 at 03:04:05PM -0400, Alan Stern wrote: > On Thu, 21 Sep 2017, Andrey Konovalov wrote: > > > On Thu, Sep 21, 2017 at 6:10 PM, Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> 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. Ugh, messy, nice catch and fix, I'll go queue it up now, thanks for resolving this. greg k-h
[toc] | [prev] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2017-09-22 10:10 +0200 |
| Message-ID | <usrc6-3JR-19@gated-at.bofh.it> |
| In reply to | #1737246 |
On Fri, Sep 22, 2017 at 09:58:15AM +0200, Greg Kroah-Hartman wrote: > On Thu, Sep 21, 2017 at 03:04:05PM -0400, Alan Stern wrote: > > On Thu, 21 Sep 2017, Andrey Konovalov wrote: > > > > > On Thu, Sep 21, 2017 at 6:10 PM, Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> 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. > > Ugh, messy, nice catch and fix, I'll go queue it up now, thanks for > resolving this. Oops, no, you didn't send this as a "real" patch yet, I'll wait :)
[toc] | [prev] | [next] | [standalone]
| From | Alan Stern <stern@rowland.harvard.edu> |
|---|---|
| Date | 2017-09-22 17:00 +0200 |
| Message-ID | <usxAS-7iO-7@gated-at.bofh.it> |
| In reply to | #1737253 |
On Fri, 22 Sep 2017, Greg Kroah-Hartman wrote: > On Fri, Sep 22, 2017 at 09:58:15AM +0200, Greg Kroah-Hartman wrote: > > On Thu, Sep 21, 2017 at 03:04:05PM -0400, Alan Stern wrote: > > > On Thu, 21 Sep 2017, Andrey Konovalov wrote: > > > > > > > On Thu, Sep 21, 2017 at 6:10 PM, Greg Kroah-Hartman > > > > <gregkh@linuxfoundation.org> 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. > > > > Ugh, messy, nice catch and fix, I'll go queue it up now, thanks for > > resolving this. > > Oops, no, you didn't send this as a "real" patch yet, I'll wait :) I was waiting for Andrey to verify that the problem was really gone. I'll submit it officially later today. On Fri, 22 Sep 2017, Felipe Balbi wrote: > > Felipe, any objection for me taking this, and the other gadget driver > > fixes that Alan just sent out, directly in my tree? > > none whatsoever, for all of them: There's still more to come. I've got four patches for dummy-hcd just about ready to send. :-) Alan Stern
[toc] | [prev] | [next] | [standalone]
| From | Andrey Konovalov <andreyknvl@google.com> |
|---|---|
| Date | 2017-09-22 13:40 +0200 |
| Message-ID | <usutj-5y4-7@gated-at.bofh.it> |
| In reply to | #1737017 |
On Thu, Sep 21, 2017 at 9:04 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, 21 Sep 2017, Andrey Konovalov wrote:
>
>> On Thu, Sep 21, 2017 at 6:10 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> 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@google.com>
>
> 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)
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web