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);
Thanks,
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 <[email protected]> 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: [email protected]
> Linux Device Driver [email protected]
> Embedded Systems phone: +39 349 2432127
> UNIX programming skype: rodolfo.giometti
On Wed, Jan 09, 2019 at 10:20:50AM +0100, Rodolfo Giometti 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?
Another option is to use check_mul_overflow().
>
> Now you have to produce a patch according to
> linux/Documentation/process/submitting-patches.rst and then submitting it!
> :-)
>
Thanks.
--
Dmitry