[PATCH] scsi: sg: update local scatter size when clamping

[PATCH] scsi: sg: update local scatter size when clamping

[Jiacheng Xu]

When scatter_elem_sz is set to a value smaller than PAGE_SIZE through
its writable sysfs module parameter, sg_build_indirect() updates the
global scatter element size variables but leaves the local num
variable unchanged.

If the parameter is set to zero, get_order(num) is called with zero,
for which its result is undefined. On 64-bit systems, the current
implementation returns an order of 64 - PAGE_SHIFT. This causes:

    ret_sz = 1 << (PAGE_SHIFT + order);

to shift a 32-bit integer by 64 bits, triggering:

    UBSAN: shift-out-of-bounds in sg_build_indirect

Update num together with the global values when clamping it to
PAGE_SIZE.

Fixes: 6460e75a104d ("[SCSI] sg: fixes for large page_size")
Signed-off-by: Jiacheng Xu <stitch@zju.edu.cn>
---
 drivers/scsi/sg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 5408f002e6c0..69013335e433 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1876,6 +1876,7 @@ 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) {
+			num = PAGE_SIZE;
 			scatter_elem_sz = PAGE_SIZE;
 			scatter_elem_sz_prev = PAGE_SIZE;
 		} else
-- 
2.25.1

Re: [PATCH] scsi: sg: update local scatter size when clamping

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: sg: compiler reload of lockless global scatter_elem_sz bypasses clamp
--

commit c196717f4b3e3f71b450481a3b45198601caf07e
Author: Jiacheng Xu <stitch@zju.edu.cn>

scsi: sg: update local scatter size when clamping

When scatter_elem_sz is set to a value smaller than PAGE_SIZE through
its writable sysfs module parameter, sg_build_indirect() updates the
global scatter element size variables but leaves the local num
variable unchanged.

> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 3f9e08725602c..64ce489128672 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -1863,6 +1863,7 @@ 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) {
> +			num = PAGE_SIZE;
>  			scatter_elem_sz = PAGE_SIZE;
>  			scatter_elem_sz_prev = PAGE_SIZE;
>  		} else

[Severity: Medium]
Since scatter_elem_sz is a global variable accessed locklessly in
sg_build_indirect(), is there a risk the compiler could reload its value
from memory after the initial read without READ_ONCE()?

If scatter_elem_sz is changed to 0 by a concurrent sysfs write right after
the `num < PAGE_SIZE` check, could a compiler reload in the `else` branch
assign 0 to scatter_elem_sz_prev?

drivers/scsi/sg.c:sg_build_indirect() {
...
		} else
			scatter_elem_sz_prev = num;
...
}

If this happens, on the next execution both variables would be 0, causing
`num != scatter_elem_sz_prev` to evaluate to false.

Would this permanently bypass the clamp and pass 0 to get_order(),
reproducing the exact shift-out-of-bounds bug this patch is attempting to
fix?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012135955.78357-1-stitch@zju.edu.cn?part=1