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
