* [PATCH] drivers: scsi: fix shift-out-of-bounds in sg_build_indirect
@ 2024-04-15 3:14 Sam Sun
2024-04-15 17:26 ` Bart Van Assche
0 siblings, 1 reply; 3+ messages in thread
From: Sam Sun @ 2024-04-15 3:14 UTC (permalink / raw
To: linux-kernel, martin.petersen
Cc: linux-scsi, xrivendell7, Bart Van Assche, syzkaller-bugs
The get_order(0) is undefined. If scatter_elem_sz is equal or below
zero, the order returned will be 52, so that PAGE_SHIFT + order is 64,
which is larger than 32 bits int range, causing shift-out-of bound.
scatter_elem_sz equals or below zero is not allowed in the
sg_build_indirect().
UBSAN: shift-out-of-bounds in /home/sy/linux-original/drivers/scsi/sg.c:1902:13
shift exponent 64 is too large for 32-bit type 'int'
CPU: 1 PID: 8078 Comm: syz-executor748 Not tainted 6.7.0-rc7 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.13.0-1ubuntu1.1 04/01/2014
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x136/0x150 lib/dump_stack.c:106
ubsan_epilogue lib/ubsan.c:217 [inline]
__ubsan_handle_shift_out_of_bounds+0x24b/0x430 lib/ubsan.c:387
sg_build_indirect.cold+0x1b/0x20 drivers/scsi/sg.c:1902
sg_build_reserve+0xc4/0x180 drivers/scsi/sg.c:2012
sg_add_sfp drivers/scsi/sg.c:2194 [inline]
sg_open+0xde4/0x1810 drivers/scsi/sg.c:350
chrdev_open+0x269/0x770 fs/char_dev.c:414
do_dentry_open+0x6d3/0x18d0 fs/open.c:948
do_open fs/namei.c:3622 [inline]
path_openat+0x1e1e/0x26d0 fs/namei.c:3779
do_filp_open+0x1c9/0x410 fs/namei.c:3809
do_sys_openat2+0x160/0x1c0 fs/open.c:1437
do_sys_open fs/open.c:1452 [inline]
__do_sys_openat fs/open.c:1468 [inline]
__se_sys_openat fs/open.c:1463 [inline]
__x64_sys_openat+0x140/0x1f0 fs/open.c:1463
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0x40/0x110 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x63/0x6b
---[ end trace ]---
Fix it by setting the minimum num to PAGE_SIZE, and change the type of
scatter_elem_sz to uint.
Reported-by: Yue Sun <samsun1006219@gmail.com>
Reported by: xingwei lee <xrivendell7@gmail.com>
Signed-off-by: Yue Sun <samsun1006219@gmail.com>
---
drivers/scsi/sg.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index 9d7b7db75e4b..6199481be585 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1586,7 +1586,7 @@ sg_remove_device(struct device *cl_dev, struct
class_interface *cl_intf)
kref_put(&sdp->d_ref, sg_device_destroy);
}
-module_param_named(scatter_elem_sz, scatter_elem_sz, int, S_IRUGO | S_IWUSR);
+module_param_named(scatter_elem_sz, scatter_elem_sz, uint, S_IRUGO | S_IWUSR);
module_param_named(def_reserved_size, def_reserved_size, int,
S_IRUGO | S_IWUSR);
module_param_named(allow_dio, sg_allow_dio, int, S_IRUGO | S_IWUSR);
@@ -1837,7 +1837,7 @@ 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;
+ num = max(scatter_elem_sz, PAGE_SIZE);
if (unlikely(num != scatter_elem_sz_prev)) {
if (num < PAGE_SIZE) {
scatter_elem_sz = PAGE_SIZE;
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: scsi: fix shift-out-of-bounds in sg_build_indirect
2024-04-15 3:14 [PATCH] drivers: scsi: fix shift-out-of-bounds in sg_build_indirect Sam Sun
@ 2024-04-15 17:26 ` Bart Van Assche
2024-04-16 6:32 ` Sam Sun
0 siblings, 1 reply; 3+ messages in thread
From: Bart Van Assche @ 2024-04-15 17:26 UTC (permalink / raw
To: Sam Sun, linux-kernel, martin.petersen
Cc: linux-scsi, xrivendell7, syzkaller-bugs
On 4/14/24 20:14, Sam Sun wrote:
> - num = scatter_elem_sz;
> + num = max(scatter_elem_sz, PAGE_SIZE);
Shouldn't the following statements be modified instead of the above
statement? I think these are the only statements that can cause
scatter_elem_sz to become smaller than PAGE_SIZE:
scatter_elem_sz = ret_sz;
scatter_elem_sz_prev = ret_sz;
Thanks,
Bart.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drivers: scsi: fix shift-out-of-bounds in sg_build_indirect
2024-04-15 17:26 ` Bart Van Assche
@ 2024-04-16 6:32 ` Sam Sun
0 siblings, 0 replies; 3+ messages in thread
From: Sam Sun @ 2024-04-16 6:32 UTC (permalink / raw
To: Bart Van Assche
Cc: linux-kernel, martin.petersen, linux-scsi, xrivendell7,
syzkaller-bugs
On Tue, Apr 16, 2024 at 1:26 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 4/14/24 20:14, Sam Sun wrote:
> > - num = scatter_elem_sz;
> > + num = max(scatter_elem_sz, PAGE_SIZE);
>
> Shouldn't the following statements be modified instead of the above
> statement? I think these are the only statements that can cause
> scatter_elem_sz to become smaller than PAGE_SIZE:
>
> scatter_elem_sz = ret_sz;
> scatter_elem_sz_prev = ret_sz;
>
Yes, these statements are the only statements that modify
scatter_elem_sz. However, ret_sz will never be less than PAGE_SIZE,
since it is calculated by 1 << (PAGE_SIZE + order), and order will not
be less than zero. So I think these statements do not need to be
modified.
scatter_elem_sz is also exported to userspace by sysfs interface
(macro module_param_named()), and privileged users could modify it to
any int they want. So I also set the exported type to uint in this
patch. Should I also change the type of scatter_elem_sz and
scatter_elem_sz_prev also to uint?
Best Regards,
Yue
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-16 6:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-15 3:14 [PATCH] drivers: scsi: fix shift-out-of-bounds in sg_build_indirect Sam Sun
2024-04-15 17:26 ` Bart Van Assche
2024-04-16 6:32 ` Sam Sun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).