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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACzwLxht_ACYD4QdDqSWfYkZ7+0a+z8DWMt15KhGCF4E1g9-Lw@mail.gmail.com>
Date: Tue, 12 Nov 2024 14:34:13 +0500
From: Sabyrzhan Tasbolatov <snovitoll@...il.com>
To: Alan Stern <stern@...land.harvard.edu>, Oliver Neukum <oneukum@...e.com>
Cc: syzbot+9760fbbd535cee131f81@...kaller.appspotmail.com, 
	gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org, 
	linux-usb@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] usb/cdc-wdm: fix memory leak of wdm_device

On Mon, Nov 11, 2024 at 7:29 PM Alan Stern <stern@...land.harvard.edu> 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@gmail.com/

>
> Alan Stern