diff mbox series

[-next,RFC] scsi: ses: fix slab-out-of-bounds in ses_enclosure_data_process

Message ID 20220713094548.3958915-1-yukuai1@huaweicloud.com
State New
Headers show
Series [-next,RFC] scsi: ses: fix slab-out-of-bounds in ses_enclosure_data_process | expand

Commit Message

Yu Kuai July 13, 2022, 9:45 a.m. UTC
From: Zhang Wensheng <zhangwensheng5@huawei.com>

Kasan report a bug like below:
[  494.865170] ==================================================================
[  494.901335] BUG: KASAN: slab-out-of-bounds in ses_enclosure_data_process+0x234/0x6f0 [ses]
[  494.901347] Write of size 1 at addr ffff8882f3181a70 by task systemd-udevd/1704
[  494.931929] i801_smbus 0000:00:1f.4: SPD Write Disable is set

[  494.944092] CPU: 12 PID: 1704 Comm: systemd-udevd Tainted: G
[  494.944101] Hardware name: Huawei 2288H V5/BC11SPSCB0, BIOS 7.01 11/13/2019
[  494.964003] i801_smbus 0000:00:1f.4: SMBus using PCI interrupt
[  494.978532] Call Trace:
[  494.978544]  dump_stack+0xbe/0xf9
[  494.978558]  print_address_description.constprop.0+0x19/0x130
[  495.092838]  ? ses_enclosure_data_process+0x234/0x6f0 [ses]
[  495.092846]  __kasan_report.cold+0x68/0x80
[  495.092855]  ? __kasan_kmalloc.constprop.0+0x71/0xd0
[  495.092862]  ? ses_enclosure_data_process+0x234/0x6f0 [ses]
[  495.092868]  kasan_report+0x3a/0x50
[  495.092875]  ses_enclosure_data_process+0x234/0x6f0 [ses]
[  495.092882]  ? mutex_unlock+0x1d/0x40
[  495.092889]  ses_intf_add+0x57f/0x910 [ses]
[  495.092900]  class_interface_register+0x26d/0x290
[  495.092906]  ? class_destroy+0xd0/0xd0
[  495.092912]  ? 0xffffffffc0bf8000
[  495.092919]  ses_init+0x18/0x1000 [ses]
[  495.092927]  do_one_initcall+0xcb/0x370
[  495.092934]  ? initcall_blacklisted+0x1b0/0x1b0
[  495.092942]  ? create_object.isra.0+0x330/0x3a0
[  495.092950]  ? kasan_unpoison_shadow+0x33/0x40
[  495.092957]  ? kasan_unpoison_shadow+0x33/0x40
[  495.092966]  do_init_module+0xe4/0x3a0
[  495.092972]  load_module+0xd0a/0xdd0
[  495.092980]  ? layout_and_allocate+0x300/0x300
[  495.092989]  ? seccomp_run_filters+0x1d6/0x2c0
[  495.092999]  ? kernel_read_file_from_fd+0xb3/0xe0
[  495.093006]  __se_sys_finit_module+0x11b/0x1b0
[  495.093012]  ? __ia32_sys_init_module+0x40/0x40
[  495.093023]  ? __audit_syscall_entry+0x226/0x290
[  495.093032]  do_syscall_64+0x33/0x40
[  495.093041]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  495.093046] RIP: 0033:0x7f39c3376089
[  495.093054] Code: 00 48 81 c4 80 00 00 00 89 f0 c3 66 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d e7 dd 0b 00 f7 d8 64 89 01 48
[  495.093058] RSP: 002b:00007ffdc6009e18 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[  495.093068] RAX: ffffffffffffffda RBX: 000055d4192801c0 RCX: 00007f39c3376089
[  495.093072] RDX: 0000000000000000 RSI: 00007f39c2fae99d RDI: 000000000000000f
[  495.093076] RBP: 00007f39c2fae99d R08: 0000000000000000 R09: 0000000000000001
[  495.093080] R10: 000000000000000f R11: 0000000000000246 R12: 0000000000000000
[  495.093084] R13: 000055d419282e00 R14: 0000000000020000 R15: 000055d41927f1f0

[  495.093091] Allocated by task 1704:
[  495.093098]  kasan_save_stack+0x1b/0x40
[  495.093105]  __kasan_kmalloc.constprop.0+0xc2/0xd0
[  495.093111]  ses_enclosure_data_process+0x65d/0x6f0 [ses]
[  495.093117]  ses_intf_add+0x57f/0x910 [ses]
[  495.093123]  class_interface_register+0x26d/0x290
[  495.093129]  ses_init+0x18/0x1000 [ses]
[  495.093134]  do_one_initcall+0xcb/0x370
[  495.093139]  do_init_module+0xe4/0x3a0
[  495.093144]  load_module+0xd0a/0xdd0
[  495.093150]  __se_sys_finit_module+0x11b/0x1b0
[  495.093155]  do_syscall_64+0x33/0x40
[  495.093162]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

[  495.093168] The buggy address belongs to the object at ffff8882f3181a40
                which belongs to the cache kmalloc-64 of size 64
[  495.093173] The buggy address is located 48 bytes inside of
                64-byte region [ffff8882f3181a40, ffff8882f3181a80)
[  495.093175] The buggy address belongs to the page:
[  495.093181] page:ffffea000bcc6000 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x2f3180
[  495.093186] head:ffffea000bcc6000 order:2 compound_mapcount:0 compound_pincount:0
[  495.093194] flags: 0x17ffe0000010200(slab|head|node=0|zone=2|lastcpupid=0x3fff)
[  495.093204] raw: 017ffe0000010200 ffffea0016e5fb08 ffffea0016921508 ffff888100050e00
[  495.093211] raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
[  495.093213] page dumped because: kasan: bad access detected

[  495.093216] Memory state around the buggy address:
[  495.093222]  ffff8882f3181900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  495.093227]  ffff8882f3181980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  495.093231] >ffff8882f3181a00: fc fc fc fc fc fc fc fc 00 00 00 00 01 fc fc fc
[  495.093234]                                                              ^
[  495.093239]  ffff8882f3181a80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  495.093244]  ffff8882f3181b00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[  495.093246] ==================================================================

After analysis on vmcore, it was found that the line "desc_ptr[len] =
'\0';" has slab-out-of-bounds problem in ses_enclosure_data_process.
In ses_enclosure_data_process, "desc_ptr" point to "buf", so it have
to be limited in the memory of "buf", however. although there is
"desc_ptr >= buf + page7_len" judgment, it does not work because
"desc_ptr + 4 + len" may bigger than "buf + page7_len", which will
lead to slab-out-of-bounds problem.

Fix it by using judging desc_ptr cross the border or not after
"desc_ptr += 4".

Signed-off-by: Zhang Wensheng <zhangwensheng5@huawei.com>
---
 drivers/scsi/ses.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Martin K. Petersen Aug. 2, 2022, 12:01 a.m. UTC | #1
> After analysis on vmcore, it was found that the line "desc_ptr[len] =
> '\0';" has slab-out-of-bounds problem in ses_enclosure_data_process.
> In ses_enclosure_data_process, "desc_ptr" point to "buf", so it have
> to be limited in the memory of "buf", however. although there is
> "desc_ptr >= buf + page7_len" judgment, it does not work because
> "desc_ptr + 4 + len" may bigger than "buf + page7_len", which will
> lead to slab-out-of-bounds problem.
>
> Fix it by using judging desc_ptr cross the border or not after
> "desc_ptr += 4".

FWIW, I tested this change and I am still getting KASAN errors from ses.
zhangwensheng (E) Aug. 2, 2022, 1:19 a.m. UTC | #2
Hi

 From my description, there is still loophole in the previous changes.
can you make a test with the following changes?

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 0a1734f34587..06b991e27c84 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -559,11 +559,11 @@ static void ses_enclosure_data_process(struct 
enclosure_device *edev,
                         struct enclosure_component *ecomp;

                         if (desc_ptr) {
-                               if (desc_ptr >= buf + page7_len) {
+                               len = (desc_ptr[2] << 8) + desc_ptr[3];
+                               desc_ptr += 4;
+                               if (desc_ptr + len > buf + page7_len) {
                                         desc_ptr = NULL;
                                 } else {
-                                       len = (desc_ptr[2] << 8) + 
desc_ptr[3];
-                                       desc_ptr += 4;
                                         /* Add trailing zero - pushes into
                                          * reserved space */
                                         desc_ptr[len] = '\0';

thanks!

Wensheng

在 2022/8/2 8:01, Martin K. Petersen 写道:
>> After analysis on vmcore, it was found that the line "desc_ptr[len] =
>> '\0';" has slab-out-of-bounds problem in ses_enclosure_data_process.
>> In ses_enclosure_data_process, "desc_ptr" point to "buf", so it have
>> to be limited in the memory of "buf", however. although there is
>> "desc_ptr >= buf + page7_len" judgment, it does not work because
>> "desc_ptr + 4 + len" may bigger than "buf + page7_len", which will
>> lead to slab-out-of-bounds problem.
>>
>> Fix it by using judging desc_ptr cross the border or not after
>> "desc_ptr += 4".
> FWIW, I tested this change and I am still getting KASAN errors from ses.
>
diff mbox series

Patch

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 0a1734f34587..981e6e950adc 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -559,11 +559,11 @@  static void ses_enclosure_data_process(struct enclosure_device *edev,
 			struct enclosure_component *ecomp;
 
 			if (desc_ptr) {
+				len = (desc_ptr[2] << 8) + desc_ptr[3];
+				desc_ptr += 4;
 				if (desc_ptr >= buf + page7_len) {
 					desc_ptr = NULL;
 				} else {
-					len = (desc_ptr[2] << 8) + desc_ptr[3];
-					desc_ptr += 4;
 					/* Add trailing zero - pushes into
 					 * reserved space */
 					desc_ptr[len] = '\0';