linux-kernel - Re: [PATCH v4] usb/cdc-wdm: fix memory info leak in wdm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <824e839d-ee72-4923-bc88-e9cc58201b07@rowland.harvard.edu>
Date: Tue, 12 Nov 2024 10:52:30 -0500
From: Alan Stern <stern@...land.harvard.edu>
To: Sabyrzhan Tasbolatov <snovitoll@...il.com>
Cc: gregkh@...uxfoundation.org, oneukum@...e.com,
	linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
	syzbot+9760fbbd535cee131f81@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH v4] usb/cdc-wdm: fix memory info leak in wdm_read

On Tue, Nov 12, 2024 at 06:29:31PM +0500, Sabyrzhan Tasbolatov wrote:
> syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> reproducer and the only report for this issue.
> 
> The check:
> 
> 	if (cntr > count)
> 		cntr = count;
> 
> only limits `cntr` to `count` (the number of bytes requested by
> userspace), but it doesn't verify that `desc->ubuf` actually has `count`
> bytes. This oversight can lead to situations where `copy_to_user` reads
> uninitialized data from `desc->ubuf`.
> 
> This patch makes sure `cntr` respects` both the `desc->length` and the
> `count` requested by userspace, preventing any uninitialized memory from
> leaking into userspace.

> ---
>  drivers/usb/class/cdc-wdm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 86ee39db013f..5a500973b463 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -598,8 +598,9 @@ static ssize_t wdm_read
>  		spin_unlock_irq(&desc->iuspin);
>  	}

Note that the code immediately before the "if" statement which ends here 
does:

	cntr = READ_ONCE(desc->length);

And the code at the end of the "if" block does:

		cntr = desc->length;

(while holding the spinlock).  Thus it is guaranteed that either way, 
cntr is equal to desc->length when we reach this point.

>  
> -	if (cntr > count)
> -		cntr = count;
> +	/* Ensure cntr does not exceed available data in ubuf. */
> +	cntr = min_t(size_t, count, desc->length);

And therefore this line does exactly the same computation as the code 
you removed.  Except for one thing: At this point the spinlock is not 
held, and your new code does not call READ_ONCE().  That is an 
oversight.

Since the new code does the same thing as the old code, it cannot 
possibly fix any bugs.

(Actually there is one other thing to watch out for: the difference 
between signed and unsigned values.  Here cntr and desc->length are 
signed whereas count is unsigned.  In theory that could cause problems 
-- it might even be related to the cause of the original bug report.  
Can you prove that desc->length will never be negative?)

Alan Stern