On Mon, Nov 11, 2024 at 7:29 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
Hello,
I've sent the v3 patch [1] per Oliver's explanation if I interpreted
it correctly.
I don't have the reproducer to verify if the patch solves the problem.
If the analysis or patch is not right, please let me know.
[1] https://lore.kernel.org/all/20241111120139.3483366-1-snovitoll@xxxxxxxxx/
>
> Alan Stern
