Re: [PATCH v4] usb/cdc-wdm: fix memory info leak in wdm_read

[Sabyrzhan Tasbolatov]

On Tue, Nov 12, 2024 at 8:52 PM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Nov 12, 2024 at 06:29:31PM +0500, Sabyrzhan Tasbolatov wrote:
> > syzbot reported "KMSAN: kernel-infoleak in wdm_read", though there is no
> > reproducer and the only report for this issue.
> >
> > The check:
> >
> >       if (cntr > count)
> >               cntr = count;
> >
> > only limits `cntr` to `count` (the number of bytes requested by
> > userspace), but it doesn't verify that `desc->ubuf` actually has `count`
> > bytes. This oversight can lead to situations where `copy_to_user` reads
> > uninitialized data from `desc->ubuf`.
> >
> > This patch makes sure `cntr` respects` both the `desc->length` and the
> > `count` requested by userspace, preventing any uninitialized memory from
> > leaking into userspace.
>
> > ---
> >  drivers/usb/class/cdc-wdm.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> > index 86ee39db013f..5a500973b463 100644
> > --- a/drivers/usb/class/cdc-wdm.c
> > +++ b/drivers/usb/class/cdc-wdm.c
> > @@ -598,8 +598,9 @@ static ssize_t wdm_read
> >               spin_unlock_irq(&desc->iuspin);
> >       }
>
> Note that the code immediately before the "if" statement which ends here
> does:
>
>         cntr = READ_ONCE(desc->length);
>
> And the code at the end of the "if" block does:
>
>                 cntr = desc->length;
>
> (while holding the spinlock).  Thus it is guaranteed that either way,
> cntr is equal to desc->length when we reach this point.
>
> >
> > -     if (cntr > count)
> > -             cntr = count;
> > +     /* Ensure cntr does not exceed available data in ubuf. */
> > +     cntr = min_t(size_t, count, desc->length);
>
> And therefore this line does exactly the same computation as the code
> you removed.  Except for one thing: At this point the spinlock is not
> held, and your new code does not call READ_ONCE().  That is an
> oversight.

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;

>
> 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);

>
> (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.

>
> Alan Stern

Please let me know if the diff makes sense and I will proceed with v5.

Thanks