* Re: [PATCH] vt: keyboard: avoid integer overflow in k_ascii
[not found] ` <20200521233138.GA5469@pizza01>
@ 2020-05-22 0:10 ` Dmitry Torokhov
0 siblings, 0 replies; 2+ messages in thread
From: Dmitry Torokhov @ 2020-05-22 0:10 UTC (permalink / raw)
To: Kyungtae Kim; +Cc: Greg KH, Jiri Slaby, syzkaller, LKML, Dave Tian
On Thu, May 21, 2020 at 11:31:39PM +0000, Kyungtae Kim wrote:
> On Wed, May 20, 2020 at 11:36:02PM -0700, Dmitry Torokhov wrote:
> > Hi,
> >
> > On Thu, May 21, 2020 at 01:34:08AM +0000, Kyungtae Kim wrote:
> > > FuzzUSB (a variant of syzkaller) found an integer overflow
> > > while processing keycode value.
> > >
> > > Reference: https://lkml.org/lkml/2020/3/22/482
> > >
> > > This bug occurs because of no validity check when operating keycode values.
> > > By executing k_ascii() multiple times, npadch can have a large value
> > > close to the max of int type e.g., 1111111111.
> > > In the following, its muliplication causes an integer overflow.
> > >
> > > This fix prevents the overflow by checking npadch using check_mul_overflow()
> > > ahead of its operation.
> > >
> > >
> > > UBSAN: Undefined behaviour in drivers/tty/vt/keyboard.c:888:19
> > > signed integer overflow:
> > > 10 * 1111111111 cannot be represented in type 'int'
> > > CPU: 0 PID: 0 Comm: swapper/0 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
> > > ubsan_epilogue+0xe/0x30 lib/ubsan.c:154
> > > handle_overflow+0xdc/0xf0 lib/ubsan.c:184
> > > __ubsan_handle_mul_overflow+0x2a/0x40 lib/ubsan.c:205
> > > k_ascii+0xbf/0xd0 drivers/tty/vt/keyboard.c:888
> > > kbd_keycode drivers/tty/vt/keyboard.c:1477 [inline]
> > > kbd_event+0x888/0x3be0 drivers/tty/vt/keyboard.c:1495
> > > input_to_handler+0x3a9/0x4b0 drivers/input/input.c:118
> > > input_pass_values.part.8+0x25e/0x690 drivers/input/input.c:145
> > > input_pass_values drivers/input/input.c:193 [inline]
> > > input_repeat_key+0x1f8/0x2c0 drivers/input/input.c:194
> > > 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:default_idle+0x2d/0x2e0 arch/x86/kernel/process.c:696
> > > Code: e5 41 57 41 56 65 44 8b 35 30 9d 5d 7a 41 55 41 54 53 0f 1f 44 00 00 e8 11 42 a4 fb e9 07 00 00 00 0f 00 2d d5 29 5e 00 fb f4 <65> 44 8b 35 0b 9d 5d 7a 0f 1f 44 00 00 5b 41 5c 41 5d 41 5e 41 5f
> > > RSP: 0018:ffffffff87007ce8 EFLAGS: 00000292 ORIG_RAX: ffffffffffffff13
> > > RAX: 0000000000000007 RBX: ffffffff87032900 RCX: 0000000000000000
> > > RDX: 0000000000000000 RSI: 0000000000000006 RDI: ffffffff87033154
> > > RBP: ffffffff87007d10 R08: fffffbfff0e06521 R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > R13: ffffffff88c99c00 R14: 0000000000000000 R15: 0000000000000000
> > > arch_cpu_idle+0xa/0x10 arch/x86/kernel/process.c:686
> > > default_idle_call+0x50/0x70 kernel/sched/idle.c:94
> > > cpuidle_idle_call kernel/sched/idle.c:154 [inline]
> > > do_idle+0x332/0x530 kernel/sched/idle.c:269
> > > cpu_startup_entry+0x18/0x20 kernel/sched/idle.c:361
> > > rest_init+0x240/0x3d0 init/main.c:660
> > > arch_call_rest_init+0xe/0x1b
> > > start_kernel+0x7f6/0x81e init/main.c:997
> > > x86_64_start_reservations+0x2a/0x2c arch/x86/kernel/head64.c:490
> > > x86_64_start_kernel+0x77/0x7a arch/x86/kernel/head64.c:471
> > > secondary_startup_64+0xa4/0xb0 arch/x86/kernel/head_64.S:242
> > >
> > >
> > > Signed-off-by: Kyungtae Kim <kt0755@gmail.com>
> > > Reported-and-tested-by: Kyungtae Kim <kt0755@gmail.com>
> > >
> > > ---
> > > drivers/tty/vt/keyboard.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > > index 15d33fa0c925..f7e1bb21bd9c 100644
> > > --- a/drivers/tty/vt/keyboard.c
> > > +++ b/drivers/tty/vt/keyboard.c
> > > @@ -869,6 +869,7 @@ static void k_meta(struct vc_data *vc, unsigned char value, char up_flag)
> > > static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
> > > {
> > > int base;
> > > + int bytes;
> > >
> > > if (up_flag)
> > > return;
> > > @@ -884,6 +885,8 @@ static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
> > >
> > > if (npadch == -1)
> > > npadch = value;
> > > + else if (check_mul_overflow(npadch, base, &bytes) || check_add_overflow(bytes, value, &bytes))
> > > + return;
> >
> > Why do we discard the result of calculation and repeat it again below?
> > Can we say
> >
> > else if (check_mul_overflow(npadch, base, &new_npadch) ||
> > check_add_overflow(new_npadch, value, &new_npadch))
Sorry, this should have been
!check...() && !check_(...)
> > npadch = new_npadch;
> >
> > Thanks.
> >
> > --
> > Dmitry
>
> Yes. This looks better.
>
> Thanks,
> Kyungtae Kim
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 2+ messages in thread