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

[Alan Stern]

On Mon, Nov 11, 2024 at 10:44:43AM +0100, Oliver Neukum wrote:
> 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.

I don't understand your analysis.  As you said, cntr is initially set to 
the amount in the buffer:

	If cntr <= count then cntr isn't changed, so the amount of data 
	copied to the user is the same as what is in the buffer.

	Otherwise, if cntr > count, then cntr is decreased so that the 
	amount copied to the user is no larger than what the user asked 
	for -- but then it's obviously smaller than what's in the buffer.

In neither case does the code copy more data than the buffer contains.

Alan Stern