linux-kernel - Re: [PATCH v4] usb/cdc-wdm: fix memory info leak in 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] [day] [month] [year] [list]

Message-ID: <CACzwLxihpmtmPgOq9tFoJB=t2QMfcrDaieAA5nuswwRsqUH3cA@mail.gmail.com>
Date: Thu, 14 Nov 2024 10:58:49 +0500
From: Sabyrzhan Tasbolatov <snovitoll@...il.com>
To: Alan Stern <stern@...land.harvard.edu>
Cc: oneukum@...e.com, gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org, 
	linux-usb@...r.kernel.org, 
	syzbot+9760fbbd535cee131f81@...kaller.appspotmail.com, 
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH v4] usb/cdc-wdm: fix memory info leak in wdm_read

On Wed, Nov 13, 2024 at 1:25 AM Alan Stern <stern@...land.harvard.edu> wrote:
>
> On Wed, Nov 13, 2024 at 12:30:08AM +0500, Sabyrzhan Tasbolatov wrote:
> > I've re-read your and Oliver's comments and come up with this diff,
> > which is the same as v4 except it is within a spinlock.
> >
> > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> > index 86ee39db013f..47b299e03e11 100644
> > --- a/drivers/usb/class/cdc-wdm.c
> > +++ b/drivers/usb/class/cdc-wdm.c
> > @@ -598,8 +598,11 @@ static ssize_t wdm_read
> >                 spin_unlock_irq(&desc->iuspin);
> >         }
> >
> > -       if (cntr > count)
> > -               cntr = count;
> > +       spin_lock_irq(&desc->iuspin);
> > +       /* Ensure cntr does not exceed available data in ubuf. */
> > +       cntr = min_t(size_t, count, desc->length);
> > +       spin_unlock_irq(&desc->iuspin);
> > +
> >         rv = copy_to_user(buffer, desc->ubuf, cntr);
> >         if (rv > 0) {
> >                 rv = -EFAULT;
>
> You seem to be stuck in a rut, doing the same thing over and over again
> and not realizing that it accomplishes nothing.  The spinlock here
> doesn't help; it merely allows you to avoid calling READ_ONCE.
>
> > > Since the new code does the same thing as the old code, it cannot
> > > possibly fix any bugs.
> >
> > Without the reproducer I can not confirm that this fixes the hypothetical bug,
> > however here is my understand how the diff above can fix the memory info leak:
> >
> > static ssize_t wdm_read() {
> >         cntr = READ_ONCE(desc->length);
> >         if (cntr == 0) {
> >                 spin_lock_irq(&desc->iuspin);
> >
> >                 /* can remain 0 if not increased in wdm_in_callback() */
> >                 cntr = desc->length;
> >
> >                 spin_unlock_irq(&desc->iuspin);
> >         }
> >
> >         spin_lock_irq(&desc->iuspin);
> >         /* take the minimum of whatever user requests `count` and
> > desc->length = 0 */
> >         cntr = min_t(size_t, count, desc->length);
> >         spin_lock_irq(&desc->iuspin);
> >
> >         /* cntr is 0, nothing to copy to the user space. */
> >         rv = copy_to_user(buffer, desc->ubuf, cntr);
>
> This does not explain anything.  How do you think your change will avoid
> the memory info leak?  That is, what differences between the old code
> and the new code will cause the leak to happen with the old code and not
> to happen with your new code?

Let me get back to this once I understand how to work with the USB gadgets
to emulate a cdc-wdm device to develop a reproducer,
because I thought that there is the path to memory info leak and
Oliver confirmed it,
but now without a solid PoC, I can't proceed further.
Sorry for the confusion.

>
> Note that if cntr is 0 then nothing is copied to user space so there is
> no info leak.
>
> > > (Actually there is one other thing to watch out for: the difference
> > > between signed and unsigned values.  Here cntr and desc->length are
> > > signed whereas count is unsigned.  In theory that could cause problems
> > > -- it might even be related to the cause of the original bug report.
> > > Can you prove that desc->length will never be negative?)
> >
> > desc->length can not be negative if I understand the following correctly:
> >
> > static void wdm_in_callback(struct urb *urb)
> > {
> >         ...
> >         int length = urb->actual_length;
> >        ...
> >        if (length + desc->length > desc->wMaxCommand) {
> >               /* The buffer would overflow */
> >              ...
> >        } else {
> >               /* we may already be in overflow */
> >               if (!test_bit(WDM_OVERFLOW, &desc->flags)) {
> >                      ...
> >                      desc->length += length;
> >                      desc->reslength = length;
> >        }
> > }
> >
> > urb->actual_length is u32, actually, need to change `int length` to
> > `u32 length` though.
>
> You don't really need to change it.  urb->actual_length can never be
> larger than urb->length.

Ack.

>
> Alan Stern