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

[Alan Stern <stern@rowland.harvard.edu>]

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?

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.

Alan Stern