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

[Alan Stern <stern@rowland.harvard.edu>]

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