On Wed, Nov 13, 2024 at 1:25 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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
