[PATCH] USB: gadget: serial: fix null pointer access in tty_wakeup()

[Kyungtae Kim]

FuzzUSB (a variant of syzkaller) found an illegal memory access
while finalizing an enumeration for a serial (acm) gadget.

Reference: https://lkml.org/lkml/2020/6/7/3

The bug arises because of accessing null instance of tty_struct
during USB enumeration phase i.e., set_config().

Although port->port.tty in gs_start_io() becomes null after gs_close() in
a separate syscall context, this tries to accesss its field (tty->flags)
in the following tty_wakeup(), which triggers the corruption.

The fix checks if port->port.tty is still valid before being used
in tty_wakeup().


BUG: KASAN: null-ptr-deref in test_bit include/asm-generic/bitops/instrumented-non-atomic.h:110 [inline]
BUG: KASAN: null-ptr-deref in tty_wakeup+0x25/0x110 drivers/tty/tty_io.c:532
Read of size 8 at addr 0000000000000460 by task systemd-udevd/2719

CPU: 2 PID: 2719 Comm: systemd-udevd Not tainted 5.6.11 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xce/0x128 lib/dump_stack.c:118
__kasan_report+0x161/0x1b0 mm/kasan/report.c:510
kasan_report+0x12/0x20 mm/kasan/common.c:641
check_memory_region_inline mm/kasan/generic.c:185 [inline]
check_memory_region+0x152/0x1b0 mm/kasan/generic.c:192
__kasan_check_read+0x11/0x20 mm/kasan/common.c:95
test_bit include/asm-generic/bitops/instrumented-non-atomic.h:110 [inline]
tty_wakeup+0x25/0x110 drivers/tty/tty_io.c:532
gs_start_io+0x1b7/0x2a0 drivers/usb/gadget/function/u_serial.c:568
gserial_connect+0x41c/0x590 drivers/usb/gadget/function/u_serial.c:1333
acm_set_alt+0x251/0x5c0 drivers/usb/gadget/function/f_acm.c:456
set_config drivers/usb/gadget/composite.c:838 [inline]
composite_setup+0x4231/0x6f10 drivers/usb/gadget/composite.c:1717
configfs_composite_setup+0x11a/0x170 drivers/usb/gadget/configfs.c:1466
dummy_timer+0xda5/0x33f0 drivers/usb/gadget/udc/dummy_hcd.c:1898
call_timer_fn+0x20e/0x770 kernel/time/timer.c:1404
expire_timers kernel/time/timer.c:1449 [inline]
__run_timers kernel/time/timer.c:1773 [inline]
run_timer_softirq+0x63f/0x13c0 kernel/time/timer.c:1786
__do_softirq+0x262/0xb46 kernel/softirq.c:292
invoke_softirq kernel/softirq.c:373 [inline]
irq_exit+0x161/0x1b0 kernel/softirq.c:413
exiting_irq arch/x86/include/asm/apic.h:546 [inline]
smp_apic_timer_interrupt+0x137/0x500 arch/x86/kernel/apic/apic.c:1146
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:829
</IRQ>
RIP: 0010:create_object+0x74c/0xba0 mm/kmemleak.c:607
Code: e9 44 fc ff ff 65 48 8b 04 25 00 0f 02 00 48 8d b8 90 04 00 00 48 ba 00 00 00 00 00 fc ff df 48 89 fe 48 c1 ee 03 0f b6 14 16 <84> d2 74 09 80 fa 03 0f 8e be 01 00 00 49 8d bf 50 01 00 00 8b 90
RSP: 0018:ffff88805ad17560 EFLAGS: 00000a02 ORIG_RAX: ffffffffffffff13
RAX: ffff88803b448000 RBX: 0000000000000120 RCX: ffffffff816e25c4
RDX: 0000000000000000 RSI: 1ffff11007689092 RDI: ffff88803b448490
RBP: ffff88805ad175b0 R08: ffffed100c9a128e R09: ffffed100c9a128e
R10: 0000000000000001 R11: ffffed100c9a128d R12: ffff888057bb8160
R13: ffff888064d09420 R14: ffff888064d09534 R15: ffff888064d093e0
kmemleak_alloc+0x21/0x30 mm/kmemleak.c:893
kmemleak_alloc_recursive include/linux/kmemleak.h:43 [inline]
slab_post_alloc_hook mm/slab.h:586 [inline]
slab_alloc_node mm/slub.c:2786 [inline]
slab_alloc mm/slub.c:2794 [inline]
kmem_cache_alloc+0x157/0x2d0 mm/slub.c:2799
__d_alloc+0x2e/0x8b0 fs/dcache.c:1690
d_alloc+0x4d/0x250 fs/dcache.c:1769
d_alloc_parallel+0xfe/0x1910 fs/dcache.c:2521
__lookup_slow+0x195/0x440 fs/namei.c:1742
lookup_slow fs/namei.c:1774 [inline]
walk_component+0x779/0xe30 fs/namei.c:1915
lookup_last fs/namei.c:2391 [inline]
path_lookupat+0x151/0x3e0 fs/namei.c:2436
filename_lookup+0x191/0x3a0 fs/namei.c:2466
user_path_at_empty+0x40/0x50 fs/namei.c:2746
user_path_at include/linux/namei.h:58 [inline]
vfs_statx+0xe9/0x190 fs/stat.c:197
vfs_lstat include/linux/fs.h:3277 [inline]
__do_sys_newlstat+0x87/0xf0 fs/stat.c:364
__se_sys_newlstat fs/stat.c:358 [inline]
__x64_sys_newlstat+0x54/0x80 fs/stat.c:358
do_syscall_64+0x9e/0x510 arch/x86/entry/common.c:294
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x7f473bb9f335
Code: 69 db 2b 00 64 c7 00 16 00 00 00 b8 ff ff ff ff c3 0f 1f 40 00 83 ff 01 48 89 f0 77 30 48 89 c7 48 89 d6 b8 06 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 03 f3 c3 90 48 8b 15 31 db 2b 00 f7 d8 64 89
RSP: 002b:00007ffc79ada6f8 EFLAGS: 00000246 ORIG_RAX: 0000000000000006
RAX: ffffffffffffffda RBX: 000055d54f102c1a RCX: 00007f473bb9f335
RDX: 00007ffc79ada7b0 RSI: 00007ffc79ada7b0 RDI: 00007ffc79ada700
RBP: 00007ffc79ada880 R08: 000000000000fc00 R09: 0000000000000000
R10: 0000000000000007 R11: 0000000000000246 R12: 00007ffc79ada890
R13: 00007ffc79ada788 R14: 0000000000000018 R15: 000055d54f846470


Signed-off-by: Kyungtae Kim <kt0...@gmail.com>
Reported-by: Kyungtae Kim <kt0...@gmail.com>

---
drivers/usb/gadget/function/u_serial.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 8167d379e115..cf4f876783d3 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -561,7 +561,7 @@ static int gs_start_io(struct gs_port *port)
port->n_read = 0;
started = gs_start_rx(port);

- if (started) {
+ if (started && port->port.tty != NULL) {
gs_start_tx(port);
/* Unblock any pending writes into our circular buffer, in case
* we didn't in gs_start_tx() */
--
2.17.1

[Jiri Slaby]

On 09. 06. 20, 7:00, Kyungtae Kim wrote:
> FuzzUSB (a variant of syzkaller) found an illegal memory access
> while finalizing an enumeration for a serial (acm) gadget.
>
> Reference: https://lkml.org/lkml/2020/6/7/3
>
> The bug arises because of accessing null instance of tty_struct
> during USB enumeration phase i.e., set_config().
>
> Although port->port.tty in gs_start_io() becomes null after gs_close() in
> a separate syscall context, this tries to accesss its field (tty->flags)
> in the following tty_wakeup(), which triggers the corruption.
>
> The fix checks if port->port.tty is still valid before being used
> in tty_wakeup().

...> Signed-off-by: Kyungtae Kim <kt0...@gmail.com>

> Reported-by: Kyungtae Kim <kt0...@gmail.com>
>
> ---
> drivers/usb/gadget/function/u_serial.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 8167d379e115..cf4f876783d3 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -561,7 +561,7 @@ static int gs_start_io(struct gs_port *port)
> port->n_read = 0;
> started = gs_start_rx(port);
>
> - if (started) {
> + if (started && port->port.tty != NULL) {

Could you explain, what prevents port->port.tty to be non-NULL on the
lines below? As far as I can see, port->port.count is set to zero at the
same place as "port->port.tty = NULL;" in gs_close. And port->port.count
is tested in the gs_start_io's caller, i.e. in gserial_connect. All this
happens under port->port_lock.

> gs_start_tx(port);
> /* Unblock any pending writes into our circular buffer, in case
> * we didn't in gs_start_tx() */

If it is a race, it will still race, only the race window would be
smaller. Or am I missing something?

The whole use of port->port.tty looks suspicious in the driver. It might
work as port->port_lock at places. But converting the uses to
tty_port_tty_set & tty_port_tty_get should fix also this problem. And
tty_port_tty_wakeup in this particular situation you are fixing. (And to
tty_port_open/close in the long run.)

thanks,
--
js
suse labs

[Kyungtae Kim]

Thanks for the comment. Yes. I agree this is sort of race bug,
and the current patch only minimizes the race window
although I thought the fix would avoid this particular memory corruption,
without affecting the consistency of program states.

So given the comment, I'm looking at this again for better patch,
but you can go ahead if you'd like to help with this.

Thanks,
Kyungtae Kim