Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

UBSAN: signed-integer-overflow in ../drivers/scsi/sr_ioctl.c:436:9 #357

Closed
JustinStitt opened this issue May 7, 2024 · 2 comments
Closed

UBSAN: signed-integer-overflow in ../drivers/scsi/sr_ioctl.c:436:9 #357

JustinStitt opened this issue May 7, 2024 · 2 comments
Assignees
@JustinStitt
Labels
[PATCH] Submitted A patch has been submitted upstream

Comments

@JustinStitt
Copy link
Collaborator 

[   65.194362] ------------[ cut here ]------------
[   65.197752] UBSAN: signed-integer-overflow in ../drivers/scsi/sr_ioctl.c:436:9
[   65.203607] -2147483648 * 177 cannot be represented in type 'int'
[   65.207911] CPU: 2 PID: 10416 Comm: syz-executor.1 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1
[   65.213585] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   65.219923] Call Trace:
[   65.221556]  <TASK>
[   65.223029]  dump_stack_lvl+0x93/0xd0
[   65.225573]  handle_overflow+0x171/0x1b0
[   65.228219]  sr_select_speed+0xeb/0xf0
[   65.230786]  ? __pm_runtime_resume+0xe6/0x130
[   65.233606]  sr_block_ioctl+0x15d/0x1d0
[   65.236132]  ? __pfx_sr_block_ioctl+0x10/0x10
[   65.238996]  blkdev_ioctl+0x419/0x500
[   65.241425]  ? __pfx_blkdev_ioctl+0x10/0x10
[   65.244138]  __se_sys_ioctl+0xdd/0x140
[   65.246642]  do_syscall_64+0xd4/0x1b0
[   65.249074]  ? arch_exit_to_user_mode_prepare+0x11/0x60
[   65.252543]  entry_SYSCALL_64_after_hwframe+0x6f/0x77
[   65.255835] RIP: 0033:0x7f10c8c05539
[   65.258150] Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 14 00 00 90 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 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
[   65.269913] RSP: 002b:00007f10c7f9d0c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   65.274946] RAX: ffffffffffffffda RBX: 00007f10c8d39f80 RCX: 00007f10c8c05539
[   65.279588] RDX: 0000000080000000 RSI: 0000000000005322 RDI: 0000000000000003
[   65.284284] RBP: 00007f10c8c64496 R08: 0000000000000000 R09: 0000000000000000
[   65.288890] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[   65.293363] R13: 0000000000000002 R14: 00007f10c8d39f80 R15: 00007fff09e25858
[   65.297865]  </TASK>
[   65.299529] ---[ end trace ]---
@JustinStitt JustinStitt self-assigned this May 7, 2024
@JustinStitt
Copy link
Collaborator Author

JustinStitt commented May 8, 2024 •
edited

patch: https://lore.kernel.org/r/20240508-b4-b4-sio-sr_select_speed-v1-1-968906b908b7@google.com

v2: https://lore.kernel.org/r/20240508-b4-b4-sio-sr_select_speed-v2-1-00b68f724290@google.com

@JustinStitt JustinStitt added the [PATCH] Submitted A patch has been submitted upstream label May 8, 2024
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue May 8, 2024
@intel-lab-lkp
scsi: sr: fix unintentional arithmetic wraparound
90690e1 
Running syzkaller with the newly reintroduced signed integer overflow
sanitizer produces this report:

[   65.194362] ------------[ cut here ]------------
[   65.197752] UBSAN: signed-integer-overflow in ../drivers/scsi/sr_ioctl.c:436:9
[   65.203607] -2147483648 * 177 cannot be represented in type 'int'
[   65.207911] CPU: 2 PID: 10416 Comm: syz-executor.1 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1
[   65.213585] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   65.219923] Call Trace:
[   65.221556]  <TASK>
[   65.223029]  dump_stack_lvl+0x93/0xd0
[   65.225573]  handle_overflow+0x171/0x1b0
[   65.228219]  sr_select_speed+0xeb/0xf0
[   65.230786]  ? __pm_runtime_resume+0xe6/0x130
[   65.233606]  sr_block_ioctl+0x15d/0x1d0
..

Historically, the signed integer overflow sanitizer did not work in the
kernel due to its interaction with `-fwrapv` but this has since been
changed [1] in the newest version of Clang. It was re-enabled in the
kernel with Commit 557f8c5 ("ubsan: Reintroduce signed overflow
sanitizer").

Let's add an extra check to make sure we don't exceed 0xffff/177 (350)
since 0xffff is the max speed. This has two benefits: 1) we deal with
integer overflow before it happens and 2) we properly respect the max
speed of 0xffff. There are some "magic" numbers here but I did not want
to change more than what was necessary.

Link: llvm/llvm-project#82432 [1]
Closes: KSPP#357
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue May 16, 2024
@martinkpetersen
scsi: sr: Fix unintentional arithmetic wraparound
9fad9d5 
Running syzkaller with the newly reintroduced signed integer overflow
sanitizer produces this report:

[   65.194362] ------------[ cut here ]------------
[   65.197752] UBSAN: signed-integer-overflow in ../drivers/scsi/sr_ioctl.c:436:9
[   65.203607] -2147483648 * 177 cannot be represented in type 'int'
[   65.207911] CPU: 2 PID: 10416 Comm: syz-executor.1 Not tainted 6.8.0-rc2-00035-gb3ef86b5a957 #1
[   65.213585] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   65.219923] Call Trace:
[   65.221556]  <TASK>
[   65.223029]  dump_stack_lvl+0x93/0xd0
[   65.225573]  handle_overflow+0x171/0x1b0
[   65.228219]  sr_select_speed+0xeb/0xf0
[   65.230786]  ? __pm_runtime_resume+0xe6/0x130
[   65.233606]  sr_block_ioctl+0x15d/0x1d0
...

Historically, the signed integer overflow sanitizer did not work in the
kernel due to its interaction with `-fwrapv` but this has since been
changed [1] in the newest version of Clang. It was re-enabled in the kernel
with Commit 557f8c5 ("ubsan: Reintroduce signed overflow sanitizer").

Firstly, let's change the type of "speed" to unsigned long as
sr_select_speed()'s only caller passes in an unsigned long anyways.

$ git grep '\.select_speed'
|	drivers/scsi/sr.c:      .select_speed           = sr_select_speed,
...
|	static int cdrom_ioctl_select_speed(struct cdrom_device_info *cdi,
|	                unsigned long arg)
|	{
|	        ...
|	        return cdi->ops->select_speed(cdi, arg);
|	}

Next, let's add an extra check to make sure we don't exceed 0xffff/177
(350) since 0xffff is the max speed. This has two benefits: 1) we deal
with integer overflow before it happens and 2) we properly respect the
max speed of 0xffff. There are some "magic" numbers here but I did not
want to change more than what was necessary.

Link: llvm/llvm-project#82432 [1]
Closes: KSPP#357
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
Link: https://lore.kernel.org/r/20240508-b4-b4-sio-sr_select_speed-v2-1-00b68f724290@google.com
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
@JustinStitt
Copy link
Collaborator Author

patch applied to a tree: https://lore.kernel.org/all/yq1msorrx04.fsf@ca-mkp.ca.oracle.com/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@JustinStitt JustinStitt

Labels
[PATCH] Submitted A patch has been submitted upstream
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@JustinStitt