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 <justinst...@google.com>
Yeah, this looks good. Thanks!
Reviewed-by: Kees Cook <keesc...@chromium.org>
--
Kees Cook
