| Message ID | 20240508-b4-b4-sio-sr_select_speed-v2-1-00b68f724290@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] scsi: sr: fix unintentional arithmetic wraparound | expand |
On Wed, May 08, 2024 at 05:22:51PM +0000, Justin Stitt wrote: > 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 557f8c582a9ba8ab ("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: https://github.com/llvm/llvm-project/pull/82432 [1] > Closes: https://github.com/KSPP/linux/issues/357 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Yeah, this looks good. Thanks! Reviewed-by: Kees Cook <keescook@chromium.org>
Justin, > 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 > ... > Applied to 6.10/scsi-staging, thanks!
On Wed, 08 May 2024 17:22:51 +0000, Justin Stitt wrote: > 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 > ... > > [...] Applied to 6.10/scsi-queue, thanks! [1/1] scsi: sr: fix unintentional arithmetic wraparound https://git.kernel.org/mkp/scsi/c/9fad9d560af5
diff --git a/Documentation/cdrom/cdrom-standard.rst b/Documentation/cdrom/cdrom-standard.rst index 7964fe134277..6c1303cff159 100644 --- a/Documentation/cdrom/cdrom-standard.rst +++ b/Documentation/cdrom/cdrom-standard.rst @@ -217,7 +217,7 @@ current *struct* is:: int (*media_changed)(struct cdrom_device_info *, int); int (*tray_move)(struct cdrom_device_info *, int); int (*lock_door)(struct cdrom_device_info *, int); - int (*select_speed)(struct cdrom_device_info *, int); + int (*select_speed)(struct cdrom_device_info *, unsigned long); int (*get_last_session) (struct cdrom_device_info *, struct cdrom_multisession *); int (*get_mcn)(struct cdrom_device_info *, struct cdrom_mcn *); @@ -396,7 +396,7 @@ action need be taken, and the return value should be 0. :: - int select_speed(struct cdrom_device_info *cdi, int speed) + int select_speed(struct cdrom_device_info *cdi, unsigned long speed) Some CD-ROM drives are capable of changing their head-speed. There are several reasons for changing the speed of a CD-ROM drive. Badly diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h index 1175f2e213b5..dc899277b3a4 100644 --- a/drivers/scsi/sr.h +++ b/drivers/scsi/sr.h @@ -65,7 +65,7 @@ int sr_disk_status(struct cdrom_device_info *); int sr_get_last_session(struct cdrom_device_info *, struct cdrom_multisession *); int sr_get_mcn(struct cdrom_device_info *, struct cdrom_mcn *); int sr_reset(struct cdrom_device_info *); -int sr_select_speed(struct cdrom_device_info *cdi, int speed); +int sr_select_speed(struct cdrom_device_info *cdi, unsigned long speed); int sr_audio_ioctl(struct cdrom_device_info *, unsigned int, void *); int sr_is_xa(Scsi_CD *); diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c index 5b0b35e60e61..a0d2556a27bb 100644 --- a/drivers/scsi/sr_ioctl.c +++ b/drivers/scsi/sr_ioctl.c @@ -425,11 +425,14 @@ int sr_reset(struct cdrom_device_info *cdi) return 0; } -int sr_select_speed(struct cdrom_device_info *cdi, int speed) +int sr_select_speed(struct cdrom_device_info *cdi, unsigned long speed) { Scsi_CD *cd = cdi->handle; struct packet_command cgc; + /* avoid exceeding the max speed or overflowing integer bounds */ + speed = clamp(0, speed, 0xffff / 177); + if (speed == 0) speed = 0xffff; /* set to max */ else diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h index 98c6fd0b39b6..fdfb61ccf55a 100644 --- a/include/linux/cdrom.h +++ b/include/linux/cdrom.h @@ -77,7 +77,7 @@ struct cdrom_device_ops { unsigned int clearing, int slot); int (*tray_move) (struct cdrom_device_info *, int); int (*lock_door) (struct cdrom_device_info *, int); - int (*select_speed) (struct cdrom_device_info *, int); + int (*select_speed) (struct cdrom_device_info *, unsigned long); int (*get_last_session) (struct cdrom_device_info *, struct cdrom_multisession *); int (*get_mcn) (struct cdrom_device_info *,
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 557f8c582a9ba8ab ("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: https://github.com/llvm/llvm-project/pull/82432 [1] Closes: https://github.com/KSPP/linux/issues/357 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Changes in v2: - change @speed type from int to unsigned long and use a clamp (thanks Kees) - also update documentation to avoid confusion - Link to v1: https://lore.kernel.org/r/20240508-b4-b4-sio-sr_select_speed-v1-1-968906b908b7@google.com --- Here's the syzkaller reproducer: r0 = openat$cdrom(0xffffffffffffff9c, &(0x7f0000000140), 0x800, 0x0) ioctl$CDROM_SELECT_SPEED(r0, 0x5322, 0x7ee9f7c1) ... which was used against Kees' tree here (v6.8rc2): https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=wip/v6.9-rc2/unsigned-overflow-sanitizer ... with this config: https://gist.github.com/JustinStitt/824976568b0f228ccbcbe49f3dee9bf4 --- Documentation/cdrom/cdrom-standard.rst | 4 ++-- drivers/scsi/sr.h | 2 +- drivers/scsi/sr_ioctl.c | 5 ++++- include/linux/cdrom.h | 2 +- 4 files changed, 8 insertions(+), 5 deletions(-) --- base-commit: 0106679839f7c69632b3b9833c3268c316c0a9fc change-id: 20240507-b4-b4-sio-sr_select_speed-e68c0d426891 Best regards, -- Justin Stitt <justinstitt@google.com>