Re: KMSAN: uninit-value in alauda_check_media

Re: KMSAN: uninit-value in alauda_check_media

[Alan Stern]

This thread has been dormant for over a year and a half.  Let's revive 
it and try to close out the issue.

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc3

Index: usb-devel/drivers/usb/storage/alauda.c
===================================================================
--- usb-devel.orig/drivers/usb/storage/alauda.c
+++ usb-devel/drivers/usb/storage/alauda.c
@@ -318,7 +318,8 @@ static int alauda_get_media_status(struc
 	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;
 }
@@ -454,9 +455,14 @@ static int alauda_init_media(struct us_d
 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;
 
-	alauda_get_media_status(us, status);
+	rc = alauda_get_media_status(us, status);
+	if (rc != USB_STOR_XFER_GOOD) {
+		status[0] = 0xF0;	/* Pretend there's no media */
+		status[1] = 0;
+	}
 
 	/* Check for no media or door open */
 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)

Re: KMSAN: uninit-value in alauda_check_media

[syzbot]

> This thread has been dormant for over a year and a half.  Let's revive 
> it and try to close out the issue.
>
> Alan Stern
>
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc3

Your commands are accepted, but please keep syzkaller-bugs@googlegroups.com mailing list in CC next time. It serves as a history of what happened with each bug report. Thank you.

>
> Index: usb-devel/drivers/usb/storage/alauda.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/storage/alauda.c
> +++ usb-devel/drivers/usb/storage/alauda.c
> @@ -318,7 +318,8 @@ static int alauda_get_media_status(struc
>  	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;
>  }
> @@ -454,9 +455,14 @@ static int alauda_init_media(struct us_d
>  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;
>  
> -	alauda_get_media_status(us, status);
> +	rc = alauda_get_media_status(us, status);
> +	if (rc != USB_STOR_XFER_GOOD) {
> +		status[0] = 0xF0;	/* Pretend there's no media */
> +		status[1] = 0;
> +	}
>  
>  	/* Check for no media or door open */
>  	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)

Re: KMSAN: uninit-value in alauda_check_media

[Alan Stern]

On Wed, Aug 02, 2023 at 09:05:05AM -0700, syzbot wrote:
> > This thread has been dormant for over a year and a half.  Let's revive 
> > it and try to close out the issue.
> >
> > Alan Stern
> >
> > #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc3
> 
> Your commands are accepted, but please keep 
> syzkaller-bugs@googlegroups.com mailing list in CC next time. It 
> serves as a history of what happened with each bug report. Thank you.

Okay.  But if it's so important to you guys that messages sent to 
syzbot+...@syzkaller.appspotmail.com are also sent to 
syzkaller-bugs@googlegroups.com, how come you haven't set up an 
automatic email forwarding service?  Wouldn't that be more reliable?

Alan Stern

Re: [syzbot] [usb?] [usb-storage?] KMSAN: uninit-value in alauda_check_media

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+e7d46eb426883fb97efd@syzkaller.appspotmail.com

Tested on:

commit:         6eaae198 Linux 6.5-rc3
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v6.5-rc3
console output: https://syzkaller.appspot.com/x/log.txt?x=159285dea80000
kernel config:  https://syzkaller.appspot.com/x/.config?x=254d7af7cd6058ea
dashboard link: https://syzkaller.appspot.com/bug?extid=e7d46eb426883fb97efd
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=1020d97ea80000

Note: testing is done by a robot and is best-effort only.

[PATCH] usb-storage: alauda: Fix uninit-value in alauda_check_media()

[Alan Stern]

Syzbot got KMSAN to complain about access to an uninitialized value in
the alauda subdriver of usb-storage:

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

The problem is that alauda_check_media() doesn't verify that its USB
transfer succeeded before trying to use the received data.  What
should happen if the transfer fails isn't entirely clear, but a
reasonably conservative approach is to pretend that no media is
present.

A similar problem exists in a usb_stor_dbg() call in
alauda_get_media_status().  In this case, when an error occurs the
call is redundant, because usb_stor_ctrl_transfer() already will print
a debugging message.

Finally, unrelated to the uninitialized memory access, is the fact
that alauda_check_media() performs DMA to a buffer on the stack.
Fortunately usb-storage provides a general purpose DMA-able buffer for
uses like this.  We'll use it instead.

Reported-and-tested-by: syzbot+e7d46eb426883fb97efd@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/0000000000007d25ff059457342d@google.com/T/
Suggested-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Fixes: e80b0fade09e ("[PATCH] USB Storage: add alauda support")
Cc: <stable@vger.kernel.org>

---

 drivers/usb/storage/alauda.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Index: usb-devel/drivers/usb/storage/alauda.c
===================================================================
--- usb-devel.orig/drivers/usb/storage/alauda.c
+++ usb-devel/drivers/usb/storage/alauda.c
@@ -318,7 +318,8 @@ static int alauda_get_media_status(struc
 	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;
 }
@@ -454,9 +455,14 @@ static int alauda_init_media(struct us_d
 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;
 
-	alauda_get_media_status(us, status);
+	rc = alauda_get_media_status(us, status);
+	if (rc != USB_STOR_XFER_GOOD) {
+		status[0] = 0xF0;	/* Pretend there's no media */
+		status[1] = 0;
+	}
 
 	/* Check for no media or door open */
 	if ((status[0] & 0x80) || ((status[0] & 0x1F) == 0x10)