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); > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html