[PATCH v2 0/2] scsi: sg: validate and clean up scatter_elem_sz handling

[PATCH v2 0/2] scsi: sg: validate and clean up scatter_elem_sz handling

[Yang Erkun]

v1->v2:
1. Split into two patches, patch 1 is the minimal bugfix that adds
validation to the scatter_elem_sz module parameter; patch 2 does the
refactoring
2. Rebased; no functional change versus v1.

v1:
https://patchwork.kernel.org/project/linux-scsi/patch/20260708063045.2008478-1-yangerkun@huawei.com/

Yang Erkun (2):
  scsi: sg: validate scatter_elem_sz module parameter
  scsi: sg: clean up scatter_elem_sz handling and module param
    permissions

 drivers/scsi/sg.c | 68 +++++++++++++++++++++++------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

-- 
2.52.0

[PATCH v2 1/2] scsi: sg: validate scatter_elem_sz module parameter

[Yang Erkun]

echo -1 or 0 > /sys/module/sg/parameters/scatter_elem_sz
exec 4<> /dev/sg0

The above triggers the following UBSAN warning:

UBSAN: shift-out-of-bounds in drivers/scsi/sg.c:1888:13
shift exponent 64 is too large for 32-bit type 'int'
.....
Call Trace:
 <TASK>
 dump_stack_lvl+0x64/0x80
 __ubsan_handle_shift_out_of_bounds+0x1d1/0x380
 sg_build_indirect.cold+0x38/0x4b
 sg_build_reserve+0x59/0x90
 sg_add_sfp+0x151/0x240
 sg_open+0x169/0x310
 chrdev_open+0xbe/0x240
 do_dentry_open+0x121/0x480
 vfs_open+0x2e/0xf0
 do_open+0x265/0x400
 path_openat+0x110/0x2b0
 do_file_open+0xe4/0x1a0
 do_sys_openat2+0x7f/0xe0
 __x64_sys_openat+0x56/0xa0
 do_syscall_64+0xf5/0x640
 entry_SYSCALL_64_after_hwframe+0x76/0x7e

The scatter_elem_sz module parameter currently lacks validation. Setting
it to -1/0 causes a left shift overflow in sg_build_indirect. Although
this overflow does not currently cause other problem, allowing
scatter_elem_sz to be set to -1/0 is not appropriate given its intended
purpose. Therefore, this patch uses module_param_call to add validation
checks: reject non-positive values and values whose order exceeds
MAX_PAGE_ORDER, and round the accepted value up to a power-of-two
multiple of PAGE_SIZE.

Fixes: 6460e75a104d ("[SCSI] sg: fixes for large page_size")
Signed-off-by: Yang Erkun <yangerkun@huawei.com>
---
 drivers/scsi/sg.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 74cd4e8a61c2..c56f8460c03f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1622,7 +1622,33 @@ sg_remove_device(struct device *cl_dev)
 	kref_put(&sdp->d_ref, sg_device_destroy);
 }
 
-module_param_named(scatter_elem_sz, scatter_elem_sz, int, S_IRUGO | S_IWUSR);
+static int scatter_elem_sz_set(const char *val, const struct kernel_param *kp)
+{
+	int ret, new_val, order;
+
+	ret = kstrtoint(val, 0, &new_val);
+	if (ret)
+		return ret;
+
+	if (new_val <= 0) {
+		pr_err("sg: scatter_elem_sz must be positive, got %d\n", new_val);
+		return -EINVAL;
+	}
+
+	order = get_order(new_val);
+	if (order > MAX_PAGE_ORDER) {
+		pr_err("sg: scatter_elem_sz too large (order %d > MAX_PAGE_ORDER %d)\n",
+			order, MAX_PAGE_ORDER);
+		return -EINVAL;
+	}
+
+	scatter_elem_sz = 1 << (PAGE_SHIFT + order);
+	return 0;
+}
+
+module_param_call(scatter_elem_sz, scatter_elem_sz_set, param_get_int,
+		  &scatter_elem_sz, 0644);
+
 module_param_named(allow_dio, sg_allow_dio, int, S_IRUGO | S_IWUSR);
 
 static int def_reserved_size_set(const char *val, const struct kernel_param *kp)
-- 
2.52.0

[PATCH v2 2/2] scsi: sg: clean up scatter_elem_sz handling and module param permissions

[Yang Erkun]

Now that scatter_elem_sz is validated and rounded up to a power-of-two
multiple of PAGE_SIZE by the .set callback at write time (and the
init-time value SG_SCATTER_SZ is always >= PAGE_SIZE), the runtime
fixup logic in sg_build_indirect() is redundant:

  - The entry fixup (syncing scatter_elem_sz_prev to scatter_elem_sz)
    was fragile -- it updated scatter_elem_sz_prev but not the local
    variable "num", which is the exact source of the UBSAN warning
    fixed by the previous patch.
  - The in-loop fixup (updating scatter_elem_sz to ret_sz when
    ret_sz > scatter_elem_sz_prev) is no longer needed because
    scatter_elem_sz is always a power-of-two multiple of PAGE_SIZE,
    making it always equal to ret_sz.
  - The init_sg boot-time clamp is also removed as it can no longer
    trigger (SG_SCATTER_SZ >= PAGE_SIZE, and the .set callback rejects
    sub-PAGE_SIZE values).

Remove the scatter_elem_sz_prev variable, the entry/in-loop fixup, and
the boot-time clamp. Snapshot scatter_elem_sz into a local elem_sz for
get_order(), and switch the debug printk to "%s"/__func__ since the
"num" argument no longer exists.

While at it, replace S_IRUGO | S_IWUSR with 0644 for the allow_dio and
def_reserved_size module parameters, as checkpatch warns about symbolic
permissions on touched module_param lines.

Signed-off-by: Yang Erkun <yangerkun@huawei.com>
---
 drivers/scsi/sg.c | 40 +++++++---------------------------------
 1 file changed, 7 insertions(+), 33 deletions(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c56f8460c03f..377c3e1dee7f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -92,7 +92,6 @@ static int def_reserved_size = SG_DEF_RESERVED_SIZE;
 static int sg_allow_dio = SG_ALLOW_DIO_DEF;
 
 static int scatter_elem_sz = SG_SCATTER_SZ;
-static int scatter_elem_sz_prev = SG_SCATTER_SZ;
 
 #define SG_SECTOR_SZ 512
 
@@ -1649,7 +1648,7 @@ static int scatter_elem_sz_set(const char *val, const struct kernel_param *kp)
 module_param_call(scatter_elem_sz, scatter_elem_sz_set, param_get_int,
 		  &scatter_elem_sz, 0644);
 
-module_param_named(allow_dio, sg_allow_dio, int, S_IRUGO | S_IWUSR);
+module_param_named(allow_dio, sg_allow_dio, int, 0644);
 
 static int def_reserved_size_set(const char *val, const struct kernel_param *kp)
 {
@@ -1675,8 +1674,7 @@ static const struct kernel_param_ops def_reserved_size_ops = {
 	.get	= param_get_int,
 };
 
-module_param_cb(def_reserved_size, &def_reserved_size_ops, &def_reserved_size,
-		   S_IRUGO | S_IWUSR);
+module_param_cb(def_reserved_size, &def_reserved_size_ops, &def_reserved_size, 0644);
 
 MODULE_AUTHOR("Douglas Gilbert");
 MODULE_DESCRIPTION("SCSI generic (sg) driver");
@@ -1694,11 +1692,6 @@ init_sg(void)
 {
 	int rc;
 
-	if (scatter_elem_sz < PAGE_SIZE) {
-		scatter_elem_sz = PAGE_SIZE;
-		scatter_elem_sz_prev = scatter_elem_sz;
-	}
-
 	rc = register_chrdev_region(MKDEV(SCSI_GENERIC_MAJOR, 0), 
 				    SG_MAX_DEVS, "sg");
 	if (rc)
@@ -1880,9 +1873,10 @@ sg_build_sgat(Sg_scatter_hold * schp, const Sg_fd * sfp, int tablesize)
 static int
 sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 {
-	int ret_sz = 0, i, k, rem_sz, num, mx_sc_elems;
+	int ret_sz = 0, i, k, rem_sz, mx_sc_elems;
 	int sg_tablesize = sfp->parentdp->sg_tablesize;
 	int blk_size = buff_size, order;
+	int elem_sz = scatter_elem_sz;
 	gfp_t gfp_mask = GFP_ATOMIC | __GFP_COMP | __GFP_NOWARN | __GFP_ZERO;
 
 	if (blk_size < 0)
@@ -1900,39 +1894,19 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, int buff_size)
 	if (mx_sc_elems < 0)
 		return mx_sc_elems;	/* most likely -ENOMEM */
 
-	num = scatter_elem_sz;
-	if (unlikely(num != scatter_elem_sz_prev)) {
-		if (num < PAGE_SIZE) {
-			scatter_elem_sz = PAGE_SIZE;
-			scatter_elem_sz_prev = PAGE_SIZE;
-		} else
-			scatter_elem_sz_prev = num;
-	}
-
-	order = get_order(num);
+	order = get_order(elem_sz);
 retry:
 	ret_sz = 1 << (PAGE_SHIFT + order);
 
 	for (k = 0, rem_sz = blk_size; rem_sz > 0 && k < mx_sc_elems;
 	     k++, rem_sz -= ret_sz) {
-
-		num = (rem_sz > scatter_elem_sz_prev) ?
-			scatter_elem_sz_prev : rem_sz;
-
 		schp->pages[k] = alloc_pages(gfp_mask, order);
 		if (!schp->pages[k])
 			goto out;
 
-		if (num == scatter_elem_sz_prev) {
-			if (unlikely(ret_sz > scatter_elem_sz_prev)) {
-				scatter_elem_sz = ret_sz;
-				scatter_elem_sz_prev = ret_sz;
-			}
-		}
-
 		SCSI_LOG_TIMEOUT(5, sg_printk(KERN_INFO, sfp->parentdp,
-				 "sg_build_indirect: k=%d, num=%d, ret_sz=%d\n",
-				 k, num, ret_sz));
+				 "%s: k=%d, ret_sz=%d\n",
+				 __func__, k, ret_sz));
 	}		/* end of for loop */
 
 	schp->page_order = order;
-- 
2.52.0