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
