Re: usb/serial/visor: slab-out-of-bounds in palm_os_3_probe
From: Andrey Konovalov
Date: Fri Sep 29 2017 - 07:35:43 EST
On Fri, Sep 29, 2017 at 10:37 AM, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Sep 28, 2017 at 07:57:46PM +0200, Andrey Konovalov wrote:
>> Hi!
>>
>> I've got the following report while fuzzing the kernel with syzkaller.
>>
>> On commit dc972a67cc54585bd83ad811c4e9b6ab3dcd427e (4.14-rc2+).
>>
>> There's no check on the connection_info->num_ports value when
>> iterating over ports.
>>
>> usb 1-1: Handspring Visor / Palm OS: port 162, is for unknown use
>> usb 1-1: Handspring Visor / Palm OS: port 81, is for unknown use
>> ==================================================================
>> BUG: KASAN: slab-out-of-bounds in palm_os_3_probe+0x4e4/0x570
>> Read of size 1 at addr ffff8800686daa26 by task kworker/0:1/24
>>
>> CPU: 0 PID: 24 Comm: kworker/0:1 Not tainted 4.14.0-rc2-42748-gcd3da2fe6c25 #324
>> 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+0x23d/0x350 mm/kasan/report.c:409
>> __asan_report_load1_noabort+0x19/0x20 mm/kasan/report.c:427
>> palm_os_3_probe+0x4e4/0x570 drivers/usb/serial/visor.c:348
>> visor_probe+0x11c/0x1e0 drivers/usb/serial/visor.c:463
>> usb_serial_probe+0x540/0x4090 drivers/usb/serial/usb-serial.c:903
>> 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:2538
>> hub_port_connect drivers/usb/core/hub.c:4984
>> hub_port_connect_change drivers/usb/core/hub.c:5090
>> port_event drivers/usb/core/hub.c:5196
>> hub_event_impl+0x1971/0x3760 drivers/usb/core/hub.c:5310
>> hub_event+0x11b/0x3f0 drivers/usb/core/hub.c:5206
>> 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 24:
>> 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_trace+0x11e/0x2d0 mm/slub.c:2772
>> kmalloc ./include/linux/slab.h:493
>> palm_os_3_probe+0x7c/0x570 drivers/usb/serial/visor.c:325
>> visor_probe+0x11c/0x1e0 drivers/usb/serial/visor.c:463
>> usb_serial_probe+0x540/0x4090 drivers/usb/serial/usb-serial.c:903
>> 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:2538
>> hub_port_connect drivers/usb/core/hub.c:4984
>> hub_port_connect_change drivers/usb/core/hub.c:5090
>> port_event drivers/usb/core/hub.c:5196
>> hub_event_impl+0x1971/0x3760 drivers/usb/core/hub.c:5310
>> hub_event+0x11b/0x3f0 drivers/usb/core/hub.c:5206
>> 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
>>
>> Freed by task 2772:
>> 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
>> signalfd_release+0x3c/0x50 fs/signalfd.c:56
>> __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+0x1d4/0x210 arch/x86/entry/common.c:162
>> prepare_exit_to_usermode arch/x86/entry/common.c:197
>> syscall_return_slowpath+0x3e2/0x4a0 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 ffff8800686daa20
>> which belongs to the cache kmalloc-8 of size 8
>> The buggy address is located 6 bytes inside of
>> 8-byte region [ffff8800686daa20, ffff8800686daa28)
>> The buggy address belongs to the page:
>> page:ffffea0001a1b680 count:1 mapcount:0 mapping: (null) index:0x0
>> flags: 0x100000000000100(slab)
>> raw: 0100000000000100 0000000000000000 0000000000000000 0000000180aa00aa
>> raw: ffffea00018f6e80 0000000500000005 ffff88006c403c80 0000000000000000
>> page dumped because: kasan: bad access detected
>>
>> Memory state around the buggy address:
>> ffff8800686da900: fb fc fc fb fc fc fb fc fc fb fc fc fb fc fc fb
>> ffff8800686da980: fc fc fb fc fc fb fc fc fb fc fc fb fc fc fb fc
>> >ffff8800686daa00: fc fb fc fc 06 fc fc fb fc fc fb fc fc fb fc fc
>> ^
>> ffff8800686daa80: fb fc fc fb fc fc fb fc fc fb fc fc fb fc fc fb
>> ffff8800686dab00: fc fc fb fc fc fb fc fc fb fc fc fb fc fc fb fc
>> ==================================================================
>
> Ah, nice catch, this bug is _old_, sorry about that.
>
> The patch below should resolve this. It looks bigger than it really is,
> as I'm just moving the error checking higher up in the function, and
> loosing an indentation for when there is invalid data.
>
> Can you let me know if this solves the issue?
Hi Greg,
I don't have a reproducer for this, but the fix looks good to me.
Reviewed-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx>
Thanks!
>
> thanks,
>
> greg k-h
>
> ------------------------
>
> diff --git a/drivers/usb/serial/visor.c b/drivers/usb/serial/visor.c
> index 9f3317a940ef..18c31bae0e10 100644
> --- a/drivers/usb/serial/visor.c
> +++ b/drivers/usb/serial/visor.c
> @@ -338,47 +338,48 @@ static int palm_os_3_probe(struct usb_serial *serial,
> goto exit;
> }
>
> - if (retval == sizeof(*connection_info)) {
> - connection_info = (struct visor_connection_info *)
> - transfer_buffer;
> -
> - num_ports = le16_to_cpu(connection_info->num_ports);
> - for (i = 0; i < num_ports; ++i) {
> - switch (
> - connection_info->connections[i].port_function_id) {
> - case VISOR_FUNCTION_GENERIC:
> - string = "Generic";
> - break;
> - case VISOR_FUNCTION_DEBUGGER:
> - string = "Debugger";
> - break;
> - case VISOR_FUNCTION_HOTSYNC:
> - string = "HotSync";
> - break;
> - case VISOR_FUNCTION_CONSOLE:
> - string = "Console";
> - break;
> - case VISOR_FUNCTION_REMOTE_FILE_SYS:
> - string = "Remote File System";
> - break;
> - default:
> - string = "unknown";
> - break;
> - }
> - dev_info(dev, "%s: port %d, is for %s use\n",
> - serial->type->description,
> - connection_info->connections[i].port, string);
> - }
> + if (retval != sizeof(*connection_info)) {
> + retval = -EINVAL;
> + goto exit;
> }
> - /*
> - * Handle devices that report invalid stuff here.
> - */
> +
> + connection_info = (struct visor_connection_info *)transfer_buffer;
> +
> + num_ports = le16_to_cpu(connection_info->num_ports);
> +
> + /* Handle devices that report invalid stuff here. */
> if (num_ports == 0 || num_ports > 2) {
> dev_warn(dev, "%s: No valid connect info available\n",
> serial->type->description);
> num_ports = 2;
> }
>
> + for (i = 0; i < num_ports; ++i) {
> + switch (
> + connection_info->connections[i].port_function_id) {
> + case VISOR_FUNCTION_GENERIC:
> + string = "Generic";
> + break;
> + case VISOR_FUNCTION_DEBUGGER:
> + string = "Debugger";
> + break;
> + case VISOR_FUNCTION_HOTSYNC:
> + string = "HotSync";
> + break;
> + case VISOR_FUNCTION_CONSOLE:
> + string = "Console";
> + break;
> + case VISOR_FUNCTION_REMOTE_FILE_SYS:
> + string = "Remote File System";
> + break;
> + default:
> + string = "unknown";
> + break;
> + }
> + dev_info(dev, "%s: port %d, is for %s use\n",
> + serial->type->description,
> + connection_info->connections[i].port, string);
> + }
> dev_info(dev, "%s: Number of ports: %d\n", serial->type->description,
> num_ports);
>