KMSAN: uninit-value in alauda_check_media

DISCUSSION syzbot 7 participants 17 messages Oct 7, 2019 – Dec 29, 2021

Subsystems (3)

USB Kernel janitors USB mass storage driver

lore

Share & tools

lore

syzbot

author syzbot

2019-10-07 19:39 UTC #0 lore raw

Hello,

syzbot found the following crash on:

HEAD commit:    1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with kmsan_i..
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
kernel config:  https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link: https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler:       clang version 9.0.0 (/home/glider/llvm/clang 80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=110631b7600000

IMPORTANT: if you fix the bug, please add the following tag to the commit:

Reported-by [email protected]

=====================================================

BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x191/0x1f0 lib/dump_stack.c:113
 kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
 __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
 alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
 alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
 usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
 usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
 usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
 kthread+0x4b5/0x4f0 kernel/kthread.c:256
 ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355

Local variable description: ----status@alauda_check_media Variable was created at:

 alauda_check_media+0x8e/0x3310 drivers/usb/storage/alauda.c:454
 alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
=====================================================
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 12279 Comm: usb-storage Tainted: G    B             5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x191/0x1f0 lib/dump_stack.c:113
 panic+0x3c9/0xc1e kernel/panic.c:219
 kmsan_report+0x2a2/0x2b0 mm/kmsan/kmsan_report.c:131
 __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
 alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
 alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
 usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
 usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
 usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
 kthread+0x4b5/0x4f0 kernel/kthread.c:256
 ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355

Kernel Offset: disabled
Rebooting in 86400 seconds..

---
This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at [email protected].

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

Jaskaran Singh

2019-10-11 11:23 UTC #1 lore raw

Quoted 82 lines · syzbot

Hello,
syzbot found the following crash on:
HEAD commit: 1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
kmsan_i..
git tree: https://github.com/google/kmsan.git master
console output:
https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
kernel config:
https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link:
https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro:
https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
C reproducer:
https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by [email protected]
=====================================================
BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0  
drivers/usb/storage/alauda.c:1137
CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0
drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Local variable description: ----status@alauda_check_media Variable was created at:
  alauda_check_media+0x8e/0x3310 drivers/usb/storage/alauda.c:454
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
=====================================================
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 12279 Comm: usb-storage Tainted:
G    B             5.3.0-rc7+  
#0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  panic+0x3c9/0xc1e kernel/panic.c:219
  kmsan_report+0x2a2/0x2b0 mm/kmsan/kmsan_report.c:131
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0
drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at [email protected].

Quoted 4 lines

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

#syz test: https://github.com/google/kmsan.git 1e76a3e5

(quoted diff) +0 -0

b/drivers/usb/storage/alauda.c
index ddab2cd3d2e7..bb309b9ad65b 100644

drivers/usb/storage/alauda.c +1 -1

@@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
 static int alauda_check_media(struct us_data *us)
 {
 	struct alauda_info *info = (struct alauda_info *) us->extra;
-	unsigned char status[2];
+	unsigned char *status = us->iobuf;
 	int rc;
 
 	rc = alauda_get_media_status(us, status);

Alexander Potapenko

2019-10-11 11:51 UTC #2 lore raw

Quoted 84 lines · Jaskaran Singh

Quoted 82 lines · syzbot
Hello,
syzbot found the following crash on:
HEAD commit: 1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
kmsan_i..
git tree: https://github.com/google/kmsan.git master
console output:
https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
kernel config:
https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link:
https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro:
https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
C reproducer:
https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by [email protected]
=====================================================
BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0
drivers/usb/storage/alauda.c:1137
CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0
drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Local variable description: ----status@alauda_check_media Variable was created at:
  alauda_check_media+0x8e/0x3310 drivers/usb/storage/alauda.c:454
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
=====================================================
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 12279 Comm: usb-storage Tainted:
G    B             5.3.0-rc7+
#0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  panic+0x3c9/0xc1e kernel/panic.c:219
  kmsan_report+0x2a2/0x2b0 mm/kmsan/kmsan_report.c:131
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0
drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at [email protected].

Quoted 6 lines

Quoted 4 lines
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
#syz test: https://github.com/google/kmsan.git 1e76a3e5

This didn't work, let's try with the master:
#syz test: https://github.com/google/kmsan.git master

Quoted 20 lines

drivers/usb/storage/alauda.c +1 -2
index ddab2cd3d2e7..bb309b9ad65b 100644
@@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
 static int alauda_check_media(struct us_data *us)
 {
        struct alauda_info *info = (struct alauda_info *) us->extra;
-       unsigned char status[2];
+       unsigned char *status = us->iobuf;
        int rc;
 
        rc = alauda_get_media_status(us, status);
 
--
You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/b8b1e4fef9f3ece63909c38b3302621d76770caa.camel%40gmail.com.

Signature · 10 lines

Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

syzbot

author syzbot

2019-10-11 15:42 UTC #3 lore raw

syzbot has tested the proposed patch but the reproducer still triggered crash: KMSAN: uninit-value in alauda_check_media

Hello,

syzbot has tested the proposed patch but the reproducer still triggered crash:
KMSAN: uninit-value in alauda_check_media

=====================================================

BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1138
CPU: 1 PID: 11015 Comm: usb-storage Not tainted 5.4.0-rc2+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x191/0x1f0 lib/dump_stack.c:113
 kmsan_report+0x14c/0x2c0 mm/kmsan/kmsan_report.c:110
 __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
 alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:461
 alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1138
 usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
 usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
 usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
 kthread+0x4b5/0x4f0 kernel/kthread.c:256
 ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355

Local variable description: ----status@alauda_check_media

Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 11015 Comm: usb-storage Tainted: G    B             5.4.0-rc2+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x191/0x1f0 lib/dump_stack.c:113
 panic+0x3c9/0xc1e kernel/panic.c:220
 kmsan_report+0x2b4/0x2c0 mm/kmsan/kmsan_report.c:133
 __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
 alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:461
 alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1138
 usb_stor_invoke_transport+0xf5/0x27e0 drivers/usb/storage/transport.c:606
 usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
 usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
 kthread+0x4b5/0x4f0 kernel/kthread.c:256
 ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355

Kernel Offset: disabled
Rebooting in 86400 seconds..

Tested on:

commit:         c40e5c97 kmsan: drop some dead code in kmsan_shadow.c
git tree:       https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=153ba453600000
kernel config:  https://syzkaller.appspot.com/x/.config?x=49548798e87d32d7
dashboard link: https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler:       clang version 9.0.0 (/home/glider/llvm/clang 80fee25776c2fb61e74c1ecb1a523375c2500b69)

Alan Stern

maintainer

2019-10-11 14:08 UTC #4 lore raw

In reply to Jaskaran Singh

Quoted 56 lines · Jaskaran Singh

Quoted 37 lines · syzbot
Hello,
syzbot found the following crash on:
HEAD commit: 1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
kmsan_i..
git tree: https://github.com/google/kmsan.git master
console output:
https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
kernel config:
https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link:
https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro:
https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
C reproducer:
https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by [email protected]
=====================================================
BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0  
drivers/usb/storage/alauda.c:1137
CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS  
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
Quoted 8 lines
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0
drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70  
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
#syz test: https://github.com/google/kmsan.git 1e76a3e5
drivers/usb/storage/alauda.c +0 -0
index ddab2cd3d2e7..bb309b9ad65b 100644

drivers/usb/storage/alauda.c static int alauda_init_media(struct us_data *us) · line 458

Quoted 8 lines

drivers/usb/storage/alauda.c +1 -1

@@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
 static int alauda_check_media(struct us_data *us)
 {
 	struct alauda_info *info = (struct alauda_info *) us->extra;
-	unsigned char status[2];
+	unsigned char *status = us->iobuf;
 	int rc;
 
 	rc = alauda_get_media_status(us, status);

That is absolutely not the correct fix.

The problem is that after this call, the code does not check rc to see if an error occurred. If there was an error, the value of status is meaningless so there's no point examining it at all.

Now yes, it's true that defining status as an array on the stack is also a bug, since USB transfer buffers are not allowed to be stack variables. And the change you made _is_ the right way to fix that bug. But that is a separate bug, not the one that syzbot found.

Alan Stern

Andrey Konovalov

2019-10-11 14:18 UTC #5 lore raw

Quoted 67 lines · Alan Stern

Quoted 64 lines · Jaskaran Singh
Quoted 37 lines · syzbot
Hello,
syzbot found the following crash on:
HEAD commit: 1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with
kmsan_i..
git tree: https://github.com/google/kmsan.git master
console output:
https://syzkaller.appspot.com/x/log.txt?x=1204cc63600000
kernel config:
https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link:
https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro:
https://syzkaller.appspot.com/x/repro.syz?x=123c860d600000
C reproducer:
https://syzkaller.appspot.com/x/repro.c?x=110631b7600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by [email protected]
=====================================================
BUG: KMSAN: uninit-value in alauda_transport+0x462/0x57f0
drivers/usb/storage/alauda.c:1137
CPU: 0 PID: 12279 Comm: usb-storage Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine,
BIOS
Google 01/01/2011
Call Trace:
  __dump_stack lib/dump_stack.c:77 [inline]
  dump_stack+0x191/0x1f0 lib/dump_stack.c:113
  kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
  __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
  alauda_check_media+0x344/0x3310 drivers/usb/storage/alauda.c:460
Quoted 8 lines
  alauda_transport+0x462/0x57f0 drivers/usb/storage/alauda.c:1137
  usb_stor_invoke_transport+0xf5/0x27e0
drivers/usb/storage/transport.c:606
  usb_stor_transparent_scsi_command+0x5d/0x70
drivers/usb/storage/protocol.c:108
  usb_stor_control_thread+0xca6/0x11a0 drivers/usb/storage/usb.c:380
  kthread+0x4b5/0x4f0 kernel/kthread.c:256
  ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
#syz test: https://github.com/google/kmsan.git 1e76a3e5
drivers/usb/storage/alauda.c +1 -1
index ddab2cd3d2e7..bb309b9ad65b 100644
@@ -452,7 +452,7 @@ static int alauda_init_media(struct us_data *us)
 static int alauda_check_media(struct us_data *us)
 {
      struct alauda_info *info = (struct alauda_info *) us->extra;
-     unsigned char status[2];
+     unsigned char *status = us->iobuf;
      int rc;
 
      rc = alauda_get_media_status(us, status);

[...]

Quoted 3 lines

Now yes, it's true that defining status as an array on the stack is also a bug, since USB transfer buffers are not allowed to be stack variables.

Hi Alan,

I'm curious, what is the reason for disallowing that? Should we try to somehow detect such cases automatically?

Thanks!

Alan Stern

maintainer

2019-10-11 14:53 UTC #6 lore raw

Quoted 10 lines · Andrey Konovalov

Quoted 3 lines · Alan Stern
Now yes, it's true that defining status as an array on the stack is also a bug, since USB transfer buffers are not allowed to be stack variables.
Hi Alan,
I'm curious, what is the reason for disallowing that? Should we try to somehow detect such cases automatically?

Transfer buffers are read and written by DMA. On systems that don't have cache-coherent DMA controllers, it is essential that the CPU does not access any cache line involved in a DMA transfer while the transfer is in progress. Otherwise the data in the cache would be different from the data in the buffer, leading to corruption.

(In theory it would be okay for the CPU to read (not write!) a cache line assigned to a buffer for a DMA write (not read!) transfer. But even doing that isn't really a good idea.)

(Also, this isn't an issue for x86 architectures, because x86 has cache-coherent DMA. But it is an issue on other architectures.)

In practice, this means transfer buffers have to be allocated by something like kmalloc, so that they occupies their own separate set of cache lines. Buffers on the stack obviously don't satisfy this requirement.

At some point there was a discussion about automatically detecting when on-stack (or otherwise invalid) buffers are used for DMA transfers. I don't recall what the outcome was.

Alan Stern

Greg Kroah-Hartman

maintainer

2019-10-11 15:06 UTC #7 lore raw

Quoted 26 lines · Alan Stern

Quoted 10 lines · Andrey Konovalov
Quoted 3 lines · Alan Stern
Now yes, it's true that defining status as an array on the stack is also a bug, since USB transfer buffers are not allowed to be stack variables.
Hi Alan,
I'm curious, what is the reason for disallowing that? Should we try to somehow detect such cases automatically?
Transfer buffers are read and written by DMA. On systems that don't have cache-coherent DMA controllers, it is essential that the CPU does not access any cache line involved in a DMA transfer while the transfer is in progress. Otherwise the data in the cache would be different from the data in the buffer, leading to corruption.
(In theory it would be okay for the CPU to read (not write!) a cache line assigned to a buffer for a DMA write (not read!) transfer. But even doing that isn't really a good idea.)
(Also, this isn't an issue for x86 architectures, because x86 has cache-coherent DMA. But it is an issue on other architectures.)

Quoted 8 lines

In practice, this means transfer buffers have to be allocated by something like kmalloc, so that they occupies their own separate set of cache lines. Buffers on the stack obviously don't satisfy this requirement.
At some point there was a discussion about automatically detecting when on-stack (or otherwise invalid) buffers are used for DMA transfers. I don't recall what the outcome was.

A patchset from Kees was sent, but it needs a bit more work...

thanks,

greg k-h

Andrey Konovalov

2019-10-14 12:56 UTC #8 lore raw

Quoted 33 lines · Greg Kroah-Hartman

Quoted 31 lines · Alan Stern
Quoted 10 lines · Andrey Konovalov
Quoted 3 lines · Alan Stern
Now yes, it's true that defining status as an array on the stack is also a bug, since USB transfer buffers are not allowed to be stack variables.
Hi Alan,
I'm curious, what is the reason for disallowing that? Should we try to somehow detect such cases automatically?
Transfer buffers are read and written by DMA. On systems that don't have cache-coherent DMA controllers, it is essential that the CPU does not access any cache line involved in a DMA transfer while the transfer is in progress. Otherwise the data in the cache would be different from the data in the buffer, leading to corruption.
(In theory it would be okay for the CPU to read (not write!) a cache line assigned to a buffer for a DMA write (not read!) transfer. But even doing that isn't really a good idea.)
(Also, this isn't an issue for x86 architectures, because x86 has cache-coherent DMA. But it is an issue on other architectures.)
In practice, this means transfer buffers have to be allocated by something like kmalloc, so that they occupies their own separate set of cache lines. Buffers on the stack obviously don't satisfy this requirement.

Quoted 5 lines

Quoted 3 lines
At some point there was a discussion about automatically detecting when on-stack (or otherwise invalid) buffers are used for DMA transfers. I don't recall what the outcome was.
A patchset from Kees was sent, but it needs a bit more work...

Hi Greg,

Could you send a link to the patchset?

Thanks!

syzbot

author syzbot

2019-10-11 15:24 UTC #9 lore raw

In reply to Jaskaran Singh

syzbot has tested the proposed patch but the reproducer still triggered crash: KMSAN: uninit-value in sd_revalidate_disk

Hello,

syzbot has tested the proposed patch but the reproducer still triggered crash:
KMSAN: uninit-value in sd_revalidate_disk

=====================================================

BUG: KMSAN: uninit-value in check_disk_change+0x423/0x4b0 fs/block_dev.c:1499
CPU: 1 PID: 23508 Comm: scsi_id Not tainted 5.3.0-rc7+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x191/0x1f0 lib/dump_stack.c:113
 kmsan_report+0x13a/0x2b0 mm/kmsan/kmsan_report.c:108
 __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:250
 media_not_present drivers/scsi/sd.c:1527 [inline]
 sd_spinup_disk drivers/scsi/sd.c:2096 [inline]
 sd_revalidate_disk+0x4d2/0xbef0 drivers/scsi/sd.c:3114
 check_disk_change+0x423/0x4b0 fs/block_dev.c:1499
 sd_open+0x471/0x8e0 drivers/scsi/sd.c:1356
 __blkdev_get+0x4a8/0x2480 fs/block_dev.c:1569
 blkdev_get+0x228/0x6d0 fs/block_dev.c:1707
 blkdev_open+0x36b/0x490 fs/block_dev.c:1846
 do_dentry_open+0xda7/0x1810 fs/open.c:797
 vfs_open+0xaf/0xe0 fs/open.c:906
 do_last fs/namei.c:3416 [inline]
 path_openat+0x17f4/0x6bb0 fs/namei.c:3533
 do_filp_open+0x2b8/0x710 fs/namei.c:3563
 do_sys_open+0x642/0xa30 fs/open.c:1089
 __do_sys_open fs/open.c:1107 [inline]
 __se_sys_open+0xad/0xc0 fs/open.c:1102
 __x64_sys_open+0x4a/0x70 fs/open.c:1102
 do_syscall_64+0xbc/0xf0 arch/x86/entry/common.c:297
 entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x7f7c9e529120
Code: 48 8b 15 1b 4d 2b 00 f7 d8 64 89 02 83 c8 ff c3 90 90 90 90 90 90 90 90 90 90 83 3d d5 a4 2b 00 00 75 10 b8 02 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 31 c3 48 83 ec 08 e8 5e 8c 01 00 48 89 04 24
RSP: 002b:00007fff97dee0a8 EFLAGS: 00000246 ORIG_RAX: 0000000000000002
RAX: ffffffffffffffda RBX: 00007fff97dee5c0 RCX: 00007f7c9e529120
RDX: 00007fff97dee3c0 RSI: 0000000000000800 RDI: 00007fff97dee3c0
RBP: 00000000017ac010 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fff97dee5c0
R13: 00007fff97dee3c0 R14: 00000000017ac010 R15: 0000000000000000

Local variable description: ----sshdr.i@sd_revalidate_disk Variable was created at:

 sd_spinup_disk drivers/scsi/sd.c:3108 [inline]
 sd_revalidate_disk+0x2d3/0xbef0 drivers/scsi/sd.c:3114
 check_disk_change+0x423/0x4b0 fs/block_dev.c:1499
=====================================================

Tested on:

commit:         1e76a3e5 kmsan: replace __GFP_NO_KMSAN_SHADOW with kmsan_i..
git tree:       https://github.com/google/kmsan.git
console output: https://syzkaller.appspot.com/x/log.txt?x=144fd0a0e00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=f03c659d0830ab8d
dashboard link: https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler:       clang version 9.0.0 (/home/glider/llvm/clang 80fee25776c2fb61e74c1ecb1a523375c2500b69)
patch:          https://syzkaller.appspot.com/x/patch.diff?x=110434ab600000

Christophe JAILLET

2021-12-28 07:47 UTC #10 lore raw

In reply to syzbot

Hi,

(2nd try - text only format - sorry for the noise)

first try to use syzbot. I hope I do it right.
Discussion about the syz report can be found at https://lore.kernel.org/linux-kernel/[email protected]/

This patch only test if alauda_get_media_status() (and its embedded usb_stor_ctrl_transfer()) before using the data. In case of error, it returns USB_STOR_TRANSPORT_ERROR as done elsewhere.

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master

drivers/usb/storage/alauda.c +6 -2

index 20b857e97e60..6c486d964911 100644
@@ -318,7 +318,8 @@ static int alauda_get_media_status(struct us_data *us, unsigned char *data)
 	rc = usb_stor_ctrl_transfer(us, us->recv_ctrl_pipe,
 		command, 0xc0, 0, 1, data, 2);
 
-	usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
+	if (rc == USB_STOR_XFER_GOOD)
+		usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
 
 	return rc;
 }

@@ -453,8 +454,11 @@ static int alauda_check_media(struct us_data *us)
 {
 	struct alauda_info *info = (struct alauda_info *) us->extra;
 	unsigned char status[2];
+	int rc;
 
-	alauda_get_media_status(us, status);
+	rc = alauda_get_media_status(us, status);
+	if (rc != USB_STOR_TRANSPORT_GOOD)
+		return USB_STOR_TRANSPORT_ERROR;
 
 	/* Check for no media or door open */
 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)

syzbot

author syzbot

2021-12-28 07:47 UTC #11 lore raw

KMSAN bugs can only be tested on https://github.com/google/kmsan.git tree because KMSAN tool is not upstreamed yet. See https://goo.gl/tpsmEJ#kmsan-bugs for details.

Quoted 9 lines

Hi,
(2nd try - text only format - sorry for the noise)
first try to use syzbot. I hope I do it right.
Discussion about the syz report can be found at
https://lore.kernel.org/linux-kernel/[email protected]/

Quoted 5 lines

This patch only test if alauda_get_media_status() (and its embedded usb_stor_ctrl_transfer()) before using the data.
In case of error, it returns USB_STOR_TRANSPORT_ERROR as done elsewhere.
#syz test:

KMSAN bugs can only be tested on https://github.com/google/kmsan.git tree because KMSAN tool is not upstreamed yet.
See https://goo.gl/tpsmEJ#kmsan-bugs for details.

Quoted 4 lines

git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
CJ

Alan Stern

maintainer

2021-12-28 22:49 UTC #12 lore raw

In reply to Christophe JAILLET

Quoted 24 lines · Christophe JAILLET

Hi,
(2nd try - text only format - sorry for the noise)
first try to use syzbot. I hope I do it right.
Discussion about the syz report can be found at
https://lore.kernel.org/linux-kernel/[email protected]/
This patch only test if alauda_get_media_status() (and its embedded usb_stor_ctrl_transfer()) before using the data.
In case of error, it returns USB_STOR_TRANSPORT_ERROR as done elsewhere.
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
CJ
drivers/usb/storage/alauda.c +0 -0
index 20b857e97e60..6c486d964911 100644

drivers/usb/storage/alauda.c static int alauda_get_media_status(struct us_data *us, unsigned char *data) · line 322

Quoted 6 lines

drivers/usb/storage/alauda.c +2 -1

@@ -318,7 +318,8 @@ static int alauda_get_media_status(struct us_data *us, unsigned char *data)
 	rc = usb_stor_ctrl_transfer(us, us->recv_ctrl_pipe,
 		command, 0xc0, 0, 1, data, 2);
 
-	usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
+	if (rc == USB_STOR_XFER_GOOD)
+		usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);

Instead of adding this test, you could initialize data[0] and data[1] to zero before the call to usb_stor_ctrl_transfer.

Quoted 6 lines

drivers/usb/storage/alauda.c +0 -0

 	return rc;
 }

@@ -453,8 +454,11 @@ static int alauda_check_media(struct us_data *us)
 {
 	struct alauda_info *info = (struct alauda_info *) us->extra;

drivers/usb/storage/alauda.c static int alauda_check_media(struct us_data *us) · line 464

Quoted 10 lines

drivers/usb/storage/alauda.c +4 -1

 	unsigned char status[2];
+	int rc;
 
-	alauda_get_media_status(us, status);
+	rc = alauda_get_media_status(us, status);
+	if (rc != USB_STOR_TRANSPORT_GOOD)
+		return USB_STOR_TRANSPORT_ERROR;
 
 	/* Check for no media or door open */
 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)

In general this looks fine. Let us know when you are ready to submit the patch.

Alan Stern

Christophe JAILLET

2021-12-29 09:16 UTC #13 lore raw

Le 28/12/2021 à 23:49, Alan Stern a écrit :

Quoted 28 lines · Christophe JAILLET

Quoted 27 lines
Hi,
(2nd try - text only format - sorry for the noise)
first try to use syzbot. I hope I do it right.
Discussion about the syz report can be found at
https://lore.kernel.org/linux-kernel/[email protected]/
This patch only test if alauda_get_media_status() (and its embedded usb_stor_ctrl_transfer()) before using the data.
In case of error, it returns USB_STOR_TRANSPORT_ERROR as done elsewhere.
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
CJ
drivers/usb/storage/alauda.c +0 -0
index 20b857e97e60..6c486d964911 100644
@@ -318,7 +318,8 @@ static int alauda_get_media_status(struct us_data *us, unsigned char *data)
  	rc = usb_stor_ctrl_transfer(us, us->recv_ctrl_pipe,
  		command, 0xc0, 0, 1, data, 2);
 

Quoted 6 lines

drivers/usb/storage/alauda.c static int alauda_get_media_status(struct us_data *us, unsigned char *data) · line 322
Quoted 3 lines
drivers/usb/storage/alauda.c +2 -1
-	usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
+	if (rc == USB_STOR_XFER_GOOD)
+		usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
Instead of adding this test, you could initialize data[0] and data[1] to zero before the call to usb_stor_ctrl_transfer.

Well, having the test is cleaner, IMHO.
If usb_stor_ctrl_transfer() fails, a message explaining the reason is already generated by the same usb_stor_dbg(). Having an error message followed by another one stating that the Media Status is 0x00 0x00 could be confusing I think.

Let me know if you have a real preference for a memset(data, 0, 2). If so, I'll add it.

Quoted 15 lines

drivers/usb/storage/alauda.c +4 -1

  	return rc;
  }

@@ -453,8 +454,11 @@ static int alauda_check_media(struct us_data *us)
  {
  	struct alauda_info *info = (struct alauda_info *) us->extra;
  	unsigned char status[2];
+	int rc;
 
-	alauda_get_media_status(us, status);
+	rc = alauda_get_media_status(us, status);
+	if (rc != USB_STOR_TRANSPORT_GOOD)
+		return USB_STOR_TRANSPORT_ERROR;

Quoted 5 lines

drivers/usb/storage/alauda.c static int alauda_check_media(struct us_data *us) · line 464
Quoted 2 lines
drivers/usb/storage/alauda.c +0 -0
  	/* Check for no media or door open */
  	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)
In general this looks fine. Let us know when you are ready to submit the patch.

I was unsure that this patch would get any interest because the driver looks old. That's why I first tried to play with syzbot :)

In the syzbot history, you also mentioned that 'unsigned char status[2]' should be 'unsigned char *status = us->iobuf;'

This is more a blind fix for me, but it looks consistent with other places that call alauda_get_media_status().

So, once you confirm if you prefer my 'if' or a 'memset', I'll resend a small serie for fixing both issues.

Quoted 3 lines

Alan Stern

Alan Stern

maintainer

2021-12-29 16:45 UTC #14 lore raw

Quoted 62 lines · Christophe JAILLET

Le 28/12/2021 à 23:49, Alan Stern a écrit :
Quoted 20 lines · Christophe JAILLET
Quoted 18 lines
Hi,
(2nd try - text only format - sorry for the noise)
first try to use syzbot. I hope I do it right.
Discussion about the syz report can be found at
https://lore.kernel.org/linux-kernel/[email protected]/
This patch only test if alauda_get_media_status() (and its embedded usb_stor_ctrl_transfer()) before using the data.
In case of error, it returns USB_STOR_TRANSPORT_ERROR as done elsewhere.
#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
CJ
Quoted 13 lines
drivers/usb/storage/alauda.c static int alauda_get_media_status(struct us_data *us, unsigned char *data) · line 321
Quoted 10 lines
drivers/usb/storage/alauda.c +2 -1
index 20b857e97e60..6c486d964911 100644
@@ -318,7 +318,8 @@ static int alauda_get_media_status(struct us_data *us, unsigned char *data)
  	rc = usb_stor_ctrl_transfer(us, us->recv_ctrl_pipe,
  		command, 0xc0, 0, 1, data, 2);
-	usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
+	if (rc == USB_STOR_XFER_GOOD)
+		usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
Instead of adding this test, you could initialize data[0] and data[1] to zero before the call to usb_stor_ctrl_transfer.
Well, having the test is cleaner, IMHO.
If usb_stor_ctrl_transfer() fails, a message explaining the reason is already generated by the same usb_stor_dbg(). Having an error message followed by another one stating that the Media Status is 0x00 0x00 could be confusing I think.
Let me know if you have a real preference for a memset(data, 0, 2). If so, I'll add it.
Quoted 17 lines
Quoted 4 lines
drivers/usb/storage/alauda.c +0 -0
  	return rc;
  }
drivers/usb/storage/alauda.c static int alauda_check_media(struct us_data *us) · line 462
Quoted 10 lines
drivers/usb/storage/alauda.c +4 -1
@@ -453,8 +454,11 @@ static int alauda_check_media(struct us_data *us)
  {
  	struct alauda_info *info = (struct alauda_info *) us->extra;
  	unsigned char status[2];
+	int rc;
-	alauda_get_media_status(us, status);
+	rc = alauda_get_media_status(us, status);
+	if (rc != USB_STOR_TRANSPORT_GOOD)
+		return USB_STOR_TRANSPORT_ERROR;
  	/* Check for no media or door open */
  	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)
In general this looks fine. Let us know when you are ready to submit the patch.

Quoted 2 lines

I was unsure that this patch would get any interest because the driver looks old. That's why I first tried to play with syzbot :)

It is indeed old. I doubt very many devices of this type are still in use.

Quoted 8 lines

In the syzbot history, you also mentioned that 'unsigned char status[2]' should be 'unsigned char *status = us->iobuf;'
This is more a blind fix for me, but it looks consistent with other places that call alauda_get_media_status().
So, once you confirm if you prefer my 'if' or a 'memset', I'll resend a small serie for fixing both issues.

"if" and "memset" are both acceptable. You can use either one.

Alan Stern

Christophe JAILLET

2021-12-28 08:01 UTC #15 lore raw

In reply to syzbot

Hi,

(3rd try - text only format, other git repo to please syzbot - sorry for the noise)

first try (ok, 3rd...) to use syzbot. I hope I do it right.
Discussion about the syz report can be found at https://lore.kernel.org/linux-kernel/0000000000007d25ff059457342d-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org/

This patch only test if alauda_get_media_status() (and its embedded usb_stor_ctrl_transfer()) before using the data. In case of error, it returns USB_STOR_TRANSPORT_ERROR as done elsewhere.

#syz test: https://github.com/google/kmsan.git master

drivers/usb/storage/alauda.c +6 -2

index 20b857e97e60..6c486d964911 100644
@@ -318,7 +318,8 @@ static int alauda_get_media_status(struct us_data *us, unsigned char *data)
 	rc = usb_stor_ctrl_transfer(us, us->recv_ctrl_pipe,
 		command, 0xc0, 0, 1, data, 2);
 
-	usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
+	if (rc == USB_STOR_XFER_GOOD)
+		usb_stor_dbg(us, "Media status %02X %02X\n", data[0], data[1]);
 
 	return rc;
 }

@@ -453,8 +454,11 @@ static int alauda_check_media(struct us_data *us)
 {
 	struct alauda_info *info = (struct alauda_info *) us->extra;
 	unsigned char status[2];
+	int rc;
 
-	alauda_get_media_status(us, status);
+	rc = alauda_get_media_status(us, status);
+	if (rc != USB_STOR_TRANSPORT_GOOD)
+		return USB_STOR_TRANSPORT_ERROR;
 
 	/* Check for no media or door open */
 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)

syzbot

author syzbot

2021-12-28 11:10 UTC #16 lore raw

syzbot has tested the proposed patch but the reproducer is still triggering an issue: KMSAN: uninit-value in scsi_mode_sense

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
KMSAN: uninit-value in scsi_mode_sense

sd 2:0:0:0: [sdd] 0 512-byte logical blocks: (0 B/0 B) sd 2:0:0:0: [sdd] 0-byte physical blocks
=====================================================

BUG: KMSAN: uninit-value in scsi_mode_sense+0x1046/0x16d0 drivers/scsi/scsi_lib.c:2200
 scsi_mode_sense+0x1046/0x16d0 drivers/scsi/scsi_lib.c:2200
 sd_revalidate_disk+0x5206/0xdfd0 drivers/scsi/sd.c:3328
 sd_probe+0x10a7/0x1970 drivers/scsi/sd.c:3567
 really_probe+0x67d/0x1510 drivers/base/dd.c:596
 __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
 driver_probe_device drivers/base/dd.c:781 [inline]
 __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
 bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
 __device_attach_async_helper+0x314/0x3e0 drivers/base/dd.c:927
 async_run_entry_fn+0xd2/0x630 kernel/async.c:127
 process_one_work+0xdb9/0x1820 kernel/workqueue.c:2298
 worker_thread+0x10bc/0x21f0 kernel/workqueue.c:2445
 kthread+0x721/0x850 kernel/kthread.c:327
 ret_from_fork+0x1f/0x30