Re: [PATCH] usb/cdc-wdm: fix memory leak of wdm_device

[Oliver Neukum <oneukum@suse.com>]

On 09.11.24 16:28, Sabyrzhan Tasbolatov wrote:

Hi,

> syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> reproducer and the only report for this issue. This might be
> a false-positive, but while the reading the code, it seems,
> there is the way to leak kernel memory.

As far as I can tell, the leak is real.

> Here what I understand so far from the report happening
> with ubuf in drivers/usb/class/cdc-wdm.c:
> 
> 1. kernel buffer "ubuf" is allocated during cdc-wdm device creation in
>     the "struct wdm_device":

Yes
[..]

> 2. during wdm_create() it calls wdm_in_callback() which MAY fill "ubuf"
>     for the first time via memmove if conditions are met.

Yes.
[..]

> 3. if conditions are not fulfilled in step 2., then calling read() syscall
>     which calls wdm_read(), should leak the random kernel memory via
>     copy_to_user() from "ubuf" buffer which is allocated in kmalloc-256.

Yes, sort of.

>   
> -	desc->ubuf = kmalloc(desc->wMaxCommand, GFP_KERNEL);
> +	desc->ubuf = kzalloc(desc->wMaxCommand, GFP_KERNEL);
>   	if (!desc->ubuf)
>   		goto err;

No. I am sorry, but the fix is wrong. Absolutely wrong.

Let's look at the code of wdm_read():

                 cntr = desc->length;
Here the method determines how much data is in the buffer.
"length" initially is zero, because the descriptor itself
is allocated with kzalloc. It is increased in the callback.

                 spin_unlock_irq(&desc->iuspin);
         }

         if (cntr > count)
                 cntr = count;

This is _supposed_ to make sure that user space does not get more
than we have in the buffer.

         rv = copy_to_user(buffer, desc->ubuf, cntr);
         if (rv > 0) {
                 rv = -EFAULT;
                 goto err;
         }

         spin_lock_irq(&desc->iuspin);

         for (i = 0; i < desc->length - cntr; i++)
                 desc->ubuf[i] = desc->ubuf[i + cntr];

         desc->length -= cntr;

Here we decrease the count of what we have in the buffer.

Now please look at the check again

"cntr" is what we have in the buffer.
"count" is how much user space wants.

We should limit what we copy to the amount we have in the buffer.
But that is not what the check does. Instead it makes sure we never
copy more than user space requested. But we do not check whether
the buffer has enough data to satisfy the read.

You have discovered the bug. If you want to propose a fix, the honor is yours.
Or do you want me to fix it?

tl;dr: Excellent catch, wrong fix

	Regards
		Oliver