On Mon, Nov 11, 2024 at 2:44 PM Oliver Neukum <oneukum@xxxxxxxx> 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.
>
> 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
Hi, thanks for the comments.
Let me try to come up with a fix with your explanation in a few hours,
this will help me understand this bug research as the complete experience.
BTW, I couldn't make a reproduction to create /dev/cdc-wdm0 device to
read from it
via dummy_hdc, USB raw gadget (learning in this part as well), to
verify the fix.
I also wanted to request a CVE for my CV
after the fix is released per kernel.org CNA rules :) but it's not so important.
>
> Regards
> Oliver
>
