Re: UBSAN: Undefined behaviour in drivers/pps/pps.c

[Kyungtae Kim]

It seems that timeout.nsec doesn't need to be patched.
But before going further, I'm just curious why such timeout variables
in the kernel
are defined as signed type variable in the first place?

Thanks,
Kyungtae Kim

On Wed, Jan 9, 2019 at 4:20 AM Rodolfo Giometti <giometti@xxxxxxxxxxxx> wrote:
>
> On 08/01/2019 21:24, Kyungtae Kim wrote:
> > We report a bug in linux-4.20: "UBSAN: Undefined behaviour in drivers/pps/pps.c"
> >
> > kernel config: https://kt0755.github.io/etc/config_v4.20_stable
> > repro: https://kt0755.github.io/etc/repro.a6372.c
> >
> > pps_cdev_pps_fetch() lacks the bounds checking for computing
> > fdata->timeout.sec * HZ, that causes such integer overflow when the result
> > is larger than the boundary.
> > The  patch below checks the possibility of overflow right before the
> > multiplication.
> >
> > =========================================
> > UBSAN: Undefined behaviour in drivers/pps/pps.c:82:30
> > signed integer overflow:
> > -7557201428062104791 * 100 cannot be represented in type 'long long int'
> > CPU: 0 PID: 10159 Comm: syz-executor6 Not tainted 4.20.0 #1
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> > Call Trace:
> >   __dump_stack lib/dump_stack.c:77 [inline]
> >   dump_stack+0xb1/0x118 lib/dump_stack.c:113
> >   ubsan_epilogue+0x12/0x94 lib/ubsan.c:159
> >   handle_overflow+0x1cf/0x21a lib/ubsan.c:190
> >   __ubsan_handle_mul_overflow+0x2a/0x35 lib/ubsan.c:214
> >   pps_cdev_pps_fetch+0x575/0x5b0 drivers/pps/pps.c:82
> >   pps_cdev_ioctl+0x567/0x910 drivers/pps/pps.c:191
> >   vfs_ioctl fs/ioctl.c:46 [inline]
> >   do_vfs_ioctl+0x1aa/0x1160 fs/ioctl.c:698
> >   ksys_ioctl+0x9e/0xb0 fs/ioctl.c:713
> >   __do_sys_ioctl fs/ioctl.c:720 [inline]
> >   __se_sys_ioctl fs/ioctl.c:718 [inline]
> >   __x64_sys_ioctl+0x7e/0xc0 fs/ioctl.c:718
> >   do_syscall_64+0xbe/0x4f0 arch/x86/entry/common.c:290
> >   entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > RIP: 0033:0x4497b9
> > Code: e8 8c 9f 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48
> > 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d
> > 01 f0 ff ff 0f 83 9b 6b fc ff c3 66 2e 0f 1f 84 00 00 00 00
> > RSP: 002b:00007f8cf875bc68 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > RAX: ffffffffffffffda RBX: 00007f8cf875c6cc RCX: 00000000004497b9
> > RDX: 0000000020000240 RSI: 00000000c00870a4 RDI: 0000000000000014
> > RBP: 000000000071bea0 R08: 0000000000000000 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
> > R13: 0000000000005c10 R14: 00000000006eecb0 R15: 00007f8cf875c700
> > =========================================
> >
> > ---
> >   drivers/pps/pps.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> > index 8febacb..66002e1 100644
> > --- a/drivers/pps/pps.c
> > +++ b/drivers/pps/pps.c
> > @@ -79,6 +79,8 @@ static int pps_cdev_pps_fetch(struct pps_device
> > *pps, struct pps_fdata *fdata)
> >                  dev_dbg(pps->dev, "timeout %lld.%09d\n",
> >                                  (long long) fdata->timeout.sec,
> >                                  fdata->timeout.nsec);
> > +               if (fdata->timeout.sec > S64_MAX / HZ)
> > +                               return -EINVAL;
> >                  ticks = fdata->timeout.sec * HZ;
> >                  ticks += fdata->timeout.nsec / (NSEC_PER_SEC / HZ);
>
> It looks good to me. Do you think is better adding a check for timeout.nsec also?
>
> Now you have to produce a patch according to
> linux/Documentation/process/submitting-patches.rst and then submitting it! :-)
>
> Ciao,
>
> Rodolfo
>
> --
> GNU/Linux Solutions                  e-mail: giometti@xxxxxxxxxxxx
> Linux Device Driver                          giometti@xxxxxxxx
> Embedded Systems                     phone:  +39 349 2432127
> UNIX programming                     skype:  rodolfo.giometti