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

[Andrey Konovalov]

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.

==================================================================
BUG: KASAN: slab-out-of-bounds in uas_probe+0x15de/0x1680
Read of size 1 at addr ffff880063ba9ad4 by task kworker/1:2/1845

CPU: 1 PID: 1845 Comm: kworker/1:2 Not tainted
4.14.0-rc1-42251-gebb2c2437d80 #215
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:16
dump_stack+0x292/0x395 lib/dump_stack.c:52
print_address_description+0x78/0x280 mm/kasan/report.c:252
kasan_report_error mm/kasan/report.c:351
kasan_report+0x22f/0x340 mm/kasan/report.c:409
__asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
uas_is_interface drivers/usb/storage/uas-detect.h:9
uas_find_uas_alt_setting drivers/usb/storage/uas-detect.h:19
uas_use_uas_driver drivers/usb/storage/uas-detect.h:64
uas_probe+0x15de/0x1680 drivers/usb/storage/uas.c:938
usb_probe_interface+0x35d/0x8e0 drivers/usb/core/driver.c:361
really_probe drivers/base/dd.c:413
driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
__device_attach_driver+0x230/0x290 drivers/base/dd.c:653
bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
__device_attach+0x26e/0x3d0 drivers/base/dd.c:710
device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
device_add+0xd0b/0x1660 drivers/base/core.c:1835
usb_set_configuration+0x104e/0x1870 drivers/usb/core/message.c:1932
generic_probe+0x73/0xe0 drivers/usb/core/generic.c:174
usb_probe_device+0xaf/0xe0 drivers/usb/core/driver.c:266
really_probe drivers/base/dd.c:413
driver_probe_device+0x610/0xa00 drivers/base/dd.c:557
__device_attach_driver+0x230/0x290 drivers/base/dd.c:653
bus_for_each_drv+0x161/0x210 drivers/base/bus.c:463
__device_attach+0x26e/0x3d0 drivers/base/dd.c:710
device_initial_probe+0x1f/0x30 drivers/base/dd.c:757
bus_probe_device+0x1eb/0x290 drivers/base/bus.c:523
device_add+0xd0b/0x1660 drivers/base/core.c:1835
usb_new_device+0x7b8/0x1020 drivers/usb/core/hub.c:2457
hub_port_connect drivers/usb/core/hub.c:4903
hub_port_connect_change drivers/usb/core/hub.c:5009
port_event drivers/usb/core/hub.c:5115
hub_event+0x194d/0x3740 drivers/usb/core/hub.c:5195
process_one_work+0xc7f/0x1db0 kernel/workqueue.c:2119
worker_thread+0x221/0x1850 kernel/workqueue.c:2253
kthread+0x3a1/0x470 kernel/kthread.c:231
ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:431

Allocated by task 2641:
save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459
kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
kmem_cache_alloc_node_trace+0x15e/0x2d0 mm/slub.c:2800
kmalloc_node ./include/linux/slab.h:531
kzalloc_node ./include/linux/slab.h:677
__get_vm_area_node+0x141/0x570 mm/vmalloc.c:1402
__vmalloc_node_range+0xa8/0x730 mm/vmalloc.c:1760
__vmalloc_node mm/vmalloc.c:1810
__vmalloc_node_flags mm/vmalloc.c:1824
vmalloc+0x4a/0x50 mm/vmalloc.c:1846
n_tty_open+0x20/0x470 drivers/tty/n_tty.c:1883
tty_ldisc_open.isra.1+0x78/0xc0 drivers/tty/tty_ldisc.c:466
tty_ldisc_setup+0x8f/0x100 drivers/tty/tty_ldisc.c:780
tty_init_dev+0x1a2/0x4a0 drivers/tty/tty_io.c:1332
ptmx_open+0xfe/0x320 drivers/tty/pty.c:831
chrdev_open+0x25c/0x710 fs/char_dev.c:416
do_dentry_open+0x74e/0xd60 fs/open.c:752
vfs_open+0x10c/0x230 fs/open.c:866
do_last fs/namei.c:3387
path_openat+0xf22/0x34d0 fs/namei.c:3527
do_filp_open+0x2a1/0x460 fs/namei.c:3562
do_sys_open+0x543/0x720 fs/open.c:1059
SYSC_open fs/open.c:1077
SyS_open+0x32/0x40 fs/open.c:1072
entry_SYSCALL_64_fastpath+0x23/0xc2 arch/x86/entry/entry_64.S:202

Freed by task 2641:
save_stack_trace+0x1b/0x20 arch/x86/kernel/stacktrace.c:59
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459
kasan_slab_free+0x72/0xc0 mm/kasan/kasan.c:524
slab_free_hook mm/slub.c:1390
slab_free_freelist_hook mm/slub.c:1412
slab_free mm/slub.c:2988
kfree+0xf6/0x2f0 mm/slub.c:3919
__vunmap+0x24c/0x2f0 mm/vmalloc.c:1545
vfree+0x55/0xe0 mm/vmalloc.c:1606
n_tty_close+0xb7/0x120 drivers/tty/n_tty.c:1864
tty_ldisc_close.isra.0+0x9e/0xd0 drivers/tty/tty_ldisc.c:490
tty_ldisc_kill+0x50/0xc0 drivers/tty/tty_ldisc.c:639
tty_ldisc_hangup+0x2a6/0x5c0 drivers/tty/tty_ldisc.c:758
__tty_hangup.part.22+0x2e6/0x690 drivers/tty/tty_io.c:612
__tty_hangup drivers/tty/tty_io.c:570
tty_vhangup+0x26/0x30 drivers/tty/tty_io.c:684
pty_close+0x36a/0x4b0 drivers/tty/pty.c:77
tty_release+0x44b/0x1520 drivers/tty/tty_io.c:1638
__fput+0x33e/0x800 fs/file_table.c:210
____fput+0x1a/0x20 fs/file_table.c:244
task_work_run+0x1af/0x280 kernel/task_work.c:112
tracehook_notify_resume ./include/linux/tracehook.h:191
exit_to_usermode_loop+0x1e1/0x220 arch/x86/entry/common.c:162
prepare_exit_to_usermode arch/x86/entry/common.c:197
syscall_return_slowpath+0x414/0x480 arch/x86/entry/common.c:266
entry_SYSCALL_64_fastpath+0xc0/0xc2 arch/x86/entry/entry_64.S:238

The buggy address belongs to the object at ffff880063ba9a80
which belongs to the cache kmalloc-64 of size 64
The buggy address is located 20 bytes to the right of
64-byte region [ffff880063ba9a80, ffff880063ba9ac0)
The buggy address belongs to the page:
page:ffffea00018eea40 count:1 mapcount:0 mapping: (null) index:0x0
flags: 0x100000000000100(slab)
raw: 0100000000000100 0000000000000000 0000000000000000 00000001802a002a
raw: ffffea0001a42280 0000000a0000000a ffff88006c403800 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff880063ba9980: 00 00 fc fc fc fc fc fc fb fb fb fb fb fb fb fb
ffff880063ba9a00: fc fc fc fc fb fb fb fb fb fb fb fb fc fc fc fc
>ffff880063ba9a80: fb fb fb fb fb fb fb fb fc fc fc fc fb fb fb fb
^
ffff880063ba9b00: fb fb fb fb fc fc fc fc 00 00 00 00 00 fc fc fc
ffff880063ba9b80: fc fc fc fc fb fb fb fb fb fb fb fb fc fc fc fc
==================================================================

[Greg Kroah-Hartman]

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".

thanks,

greg k-h

[Andrey Konovalov]

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.

>
> thanks,
>
> greg k-h

[Alan Stern]

On Thu, 21 Sep 2017, 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.

My guess is the latter, although I can't see what is going wrong. Can
you provide the code that does this?

Alan Stern

[Andrey Konovalov]

I did, see the previous email (replying in case you missed it).

>
> Alan Stern
>

[Alan Stern]

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)

[Greg Kroah-Hartman]

Ugh, messy, nice catch and fix, I'll go queue it up now, thanks for
resolving this.

greg k-h

[Greg Kroah-Hartman]

Oops, no, you didn't send this as a "real" patch yet, I'll wait :)

[Andrey Konovalov]

On Thu, Sep 21, 2017 at 9:04 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Thu, 21 Sep 2017, Andrey Konovalov wrote:
>
>> On Thu, Sep 21, 2017 at 6:10 PM, Greg Kroah-Hartman
>> <gre...@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 <andre...@google.com>

[Alan Stern]

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