Re: [PATCH] scsi: sg: Fix shift-out-of-bounds in sg_build_indirect()

[Krystian Kaniewski <krystianmkaniewski@gmail.com>]

On 9/17/26 10:04, syzbot wrote:
> From: Krystian Kaniewski <krystianmkaniewski@gmail.com>
> 
> Writing an invalid value such as 0 or a negative number to
> /sys/module/sg/parameters/scatter_elem_sz, or setting it at module load
> time, causes a shift-out-of-bounds bug during subsequent device operations.
> In init_sg(), scatter_elem_sz is checked against PAGE_SIZE, but because
> PAGE_SIZE is unsigned, the comparison is unsigned and bypasses negative
> values. When opening a device via sg_open(), the driver calls
> sg_build_indirect() to allocate scatter-gather buffers. In
> sg_build_indirect(), scatter_elem_sz is read into the local snapshot
> variable num. Because PAGE_SIZE is unsigned, comparing num against
> PAGE_SIZE also results in an unsigned comparison that bypasses negative
> values. Furthermore, even if the condition is met, the code updates the
> global scatter_elem_sz and scatter_elem_sz_prev variables but leaves the
> local snapshot variable num unupdated before get_order(). As a result, an
> invalid value such as 0 remains in num and is passed to order =
> get_order(num). Calling get_order(0) is undefined and underflows to 52 on
> 64-bit systems, causing ret_sz = 1 << (PAGE_SHIFT + order) to compute 1 <<
> 64. Shifting a 32-bit int by 64 bits triggers a UBSAN shift-out-of-bounds
> warning:
> 
> UBSAN: shift-out-of-bounds in drivers/scsi/sg.c:1887:13
> shift exponent 64 is too large for 32-bit type 'int'
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
>  ubsan_epilogue+0xa/0x30 lib/ubsan.c:233
>  __ubsan_handle_shift_out_of_bounds+0x36d/0x400 lib/ubsan.c:494
>  sg_build_indirect+0x559/0x870 drivers/scsi/sg.c:1887
>  sg_build_reserve drivers/scsi/sg.c:1997 [inline]
>  sg_add_sfp drivers/scsi/sg.c:2177 [inline]
>  sg_open+0x1128/0x18a0 drivers/scsi/sg.c:349
>  chrdev_open+0x4d9/0x600 fs/char_dev.c:411
>  do_dentry_open+0x816/0x1380 fs/open.c:996
>  vfs_open+0x3b/0x340 fs/open.c:1101
>  </TASK>
> 
> Fix this by casting PAGE_SIZE to int in both init_sg() and
> sg_build_indirect() so that the comparisons are signed and negative values
> do not bypass the check, and by updating the local snapshot variable num to
> PAGE_SIZE when num < (int)PAGE_SIZE in sg_build_indirect() before
> get_order() is called.
> 
> Fixes: 10db10d144c0 ("sg: convert the indirect IO path to use the block layer")
> Assisted-by: Gemini:gemini-3.8-flash syzbot
> Reported-by: syzbot+270f1c719ee7baab9941@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=270f1c719ee7baab9941
> Link: https://syzkaller.appspot.com/ai_job?id=1d412b83-5bbc-4d73-b690-4c798d45cee4
> Signed-off-by: Krystian Kaniewski <krystianmkaniewski@gmail.com>
> 
> ---
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 5408f002e..46117d1a1 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1667,7 +1667,7 @@ init_sg(void)
>  {
>  	int rc;
>  
> -	if (scatter_elem_sz < PAGE_SIZE) {
> +	if (scatter_elem_sz < (int)PAGE_SIZE) {
>  		scatter_elem_sz = PAGE_SIZE;
>  		scatter_elem_sz_prev = scatter_elem_sz;
>  	}
> @@ -1875,9 +1875,10 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
>  
>  	num = scatter_elem_sz;
>  	if (unlikely(num != scatter_elem_sz_prev)) {
> -		if (num < PAGE_SIZE) {
> +		if (num < (int)PAGE_SIZE) {
>  			scatter_elem_sz = PAGE_SIZE;
>  			scatter_elem_sz_prev = PAGE_SIZE;
> +			num = PAGE_SIZE;
>  		} else
>  			scatter_elem_sz_prev = num;
>  	}
> 
> 
> base-commit: df2908090cda368b01ff43709f51890076c56157
Hi Martin, Doug,

A gentle ping on this patch. Patchwork currently shows it as deferred.
Is there anything you would like me to change, or should I simply leave
it pending?

Thanks,
-- 
Krystian Kaniewski