Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

scsi: sr: Fix unintentional arithmetic wraparound

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 557f8c582a9b ("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>
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>

authored by

Justin Stitt and committed by
Martin K. Petersen
9fad9d56 10157b1f

+8 -5
+2 -2
Documentation/cdrom/cdrom-standard.rst
··· 217 217 int (*media_changed)(struct cdrom_device_info *, int); 218 218 int (*tray_move)(struct cdrom_device_info *, int); 219 219 int (*lock_door)(struct cdrom_device_info *, int); 220 - int (*select_speed)(struct cdrom_device_info *, int); 220 + int (*select_speed)(struct cdrom_device_info *, unsigned long); 221 221 int (*get_last_session) (struct cdrom_device_info *, 222 222 struct cdrom_multisession *); 223 223 int (*get_mcn)(struct cdrom_device_info *, struct cdrom_mcn *); ··· 396 396 397 397 :: 398 398 399 - int select_speed(struct cdrom_device_info *cdi, int speed) 399 + int select_speed(struct cdrom_device_info *cdi, unsigned long speed) 400 400 401 401 Some CD-ROM drives are capable of changing their head-speed. There 402 402 are several reasons for changing the speed of a CD-ROM drive. Badly
+1 -1
drivers/scsi/sr.h
··· 65 65 int sr_get_last_session(struct cdrom_device_info *, struct cdrom_multisession *); 66 66 int sr_get_mcn(struct cdrom_device_info *, struct cdrom_mcn *); 67 67 int sr_reset(struct cdrom_device_info *); 68 - int sr_select_speed(struct cdrom_device_info *cdi, int speed); 68 + int sr_select_speed(struct cdrom_device_info *cdi, unsigned long speed); 69 69 int sr_audio_ioctl(struct cdrom_device_info *, unsigned int, void *); 70 70 71 71 int sr_is_xa(Scsi_CD *);
+4 -1
drivers/scsi/sr_ioctl.c
··· 425 425 return 0; 426 426 } 427 427 428 - int sr_select_speed(struct cdrom_device_info *cdi, int speed) 428 + int sr_select_speed(struct cdrom_device_info *cdi, unsigned long speed) 429 429 { 430 430 Scsi_CD *cd = cdi->handle; 431 431 struct packet_command cgc; 432 + 433 + /* avoid exceeding the max speed or overflowing integer bounds */ 434 + speed = clamp(0, speed, 0xffff / 177); 432 435 433 436 if (speed == 0) 434 437 speed = 0xffff; /* set to max */
+1 -1
include/linux/cdrom.h
··· 77 77 unsigned int clearing, int slot); 78 78 int (*tray_move) (struct cdrom_device_info *, int); 79 79 int (*lock_door) (struct cdrom_device_info *, int); 80 - int (*select_speed) (struct cdrom_device_info *, int); 80 + int (*select_speed) (struct cdrom_device_info *, unsigned long); 81 81 int (*get_last_session) (struct cdrom_device_info *, 82 82 struct cdrom_multisession *); 83 83 int (*get_mcn) (struct cdrom_device_info *,