usb/serial/visor: slab-out-of-bounds in palm_os_3_probe

[Andrey Konovalov]

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

[Greg Kroah-Hartman]

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?

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

[Andrey Konovalov]

Hi Greg,

I don't have a reproducer for this, but the fix looks good to me.

Reviewed-by: Andrey Konovalov <andre...@google.com>

Thanks!

[Johan Hovold]

On Fri, Sep 29, 2017 at 10:37:55AM +0200, Greg Kroah-Hartman 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

Thanks for the report, Andrey.

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

And thanks for fixing this up, Greg. Will you send a proper patch that I
can apply?

A couple of comments below.

Since we're refusing to bind here -ENODEV may be more appropriate. And
I think a dev_err() (or dev_warn()) is warranted too.

> + goto exit;

So far we have not been bailing probe when the returned connection-info size
wasn't the expected one (we'd only skip the dev_info() above). I'm sure
this is fine, but the tightened check could potentially cause issues if
there are devices returning additional fields.

> }
> - /*
> - * 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) {

Perhaps you can merge the above two lines now while at it.

Thanks,
Johan

[Greg Kroah-Hartman]

On Tue, Oct 03, 2017 at 11:29:40AM +0200, Johan Hovold wrote:
> On Fri, Sep 29, 2017 at 10:37:55AM +0200, Greg Kroah-Hartman 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
>
> Thanks for the report, Andrey.
>
> > 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?
>
> And thanks for fixing this up, Greg. Will you send a proper patch that I
> can apply?

Yes, let me redo it based on your comments, and will send it out
"correctly" in a few days.

thanks for the review,

greg k-h

[Andrey Konovalov]

Hi Greg,

I was going through the bugs I've reported, and it seems that you
didn't mail the patch for this one. Reminding in case you've
accidentally forgotten about it.

Thanks!

[Greg Kroah-Hartman]

It's not forgotten, it's on my TODO list, sorry, been swamped with other
things these past few weeks. Hope to get to it soon...

thanks,

greg k-h