[Bug 220981] New: Potential data race in drivers/usb/class/cdc-wdm.c::wdm_read

[Bug 220981] New: Potential data race in drivers/usb/class/cdc-wdm.c::wdm_read

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=220981

            Bug ID: 220981
           Summary: Potential data race in
                    drivers/usb/class/cdc-wdm.c::wdm_read
           Product: Drivers
           Version: 2.5
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: low
          Priority: P3
         Component: USB
          Assignee: drivers_usb@kernel-bugs.kernel.org
          Reporter: franci.vi@tiscali.it
        Regression: No

This issue is related to https://bugzilla.kernel.org/show_bug.cgi?id=220857
where a tool was used to detect possible TOCTOUs.
A list of affected source files is attached there: among those
drivers/usb/class/cdc-wdm.c


By looking at wdm_read() implementation:
wdm_read() copies data from desc->ubuf to userspace through copy_to_user(),
without holding the spin-lock desc->iuspin,
while the RX completion handler wdm_in_callback() may simultaneously write
desc->ubuf under the same spin-lock.


A possible fix could be:

1) grab desc->iuspin before we get the amount of data desc->length and before
we access desc->ubuf.
2) copy the requested bytes into a temporary kernel buffer while still
holding the spin-lock, then update desc->ubuf/desc->length.
3) drop the spin-lock and perform the copy_to_user().
Handle the case where the buffer desc->ubuf became empty after we took the
lock.

Below the full implementation of wdm_read().

static ssize_t wdm_read
(struct file *file, char __user *buffer, size_t count, loff_t *ppos)
{
        int rv, cntr;
        int i = 0;
        struct wdm_device *desc = file->private_data;
        u8 *kbuf = NULL;                /* temporary copy of data */

again:
        rv = mutex_lock_interruptible(&desc->rlock); /* concurrent reads */
        if (rv < 0)
                return -ERESTARTSYS;

        cntr = READ_ONCE(desc->length);
        if (cntr == 0) {
                desc->read = 0;
retry:
                if (test_bit(WDM_DISCONNECTING, &desc->flags)) {
                        rv = -ENODEV;
                        goto err;
                }
                if (test_bit(WDM_OVERFLOW, &desc->flags)) {
                        clear_bit(WDM_OVERFLOW, &desc->flags);
                        rv = -ENOBUFS;
                        goto err;
                }
                i++;
                if (file->f_flags & O_NONBLOCK) {
                        if (!test_bit(WDM_READ, &desc->flags)) {
                                rv = -EAGAIN;
                                goto err;
                        }
                        rv = 0;
                } else {
                        rv = wait_event_interruptible(desc->wait,
                                test_bit(WDM_READ, &desc->flags));
                }

                /* may have happened while we slept */
                if (test_bit(WDM_DISCONNECTING, &desc->flags)) {
                        rv = -ENODEV;
                        goto err;
                }
                if (test_bit(WDM_RESETTING, &desc->flags)) {
                        rv = -EIO;
                        goto err;
                }
                usb_mark_last_busy(interface_to_usbdev(desc->intf));
                if (rv < 0) {
                        rv = -ERESTARTSYS;
                        goto err;
                }

                spin_lock_irq(&desc->iuspin);

                if (desc->rerr) { /* read completed, error happened */
                        rv = usb_translate_errors(desc->rerr);
                        desc->rerr = 0;
                        spin_unlock_irq(&desc->iuspin);
                        goto err;
                }
                /*
                 * recheck whether we've lost the race
                 * against the completion handler
                 */
                if (!test_bit(WDM_READ, &desc->flags)) { /* lost race */
                        spin_unlock_irq(&desc->iuspin);
                        goto retry;
                }

                cntr = desc->length;
                spin_unlock_irq(&desc->iuspin);
        }

        if (cntr > count)
                cntr = count;

        /* allocate temporary buffer before taking spinlock */
        kbuf = kmalloc(cntr, GFP_KERNEL);
        if (!kbuf) {
                rv = -ENOMEM;
                goto err;
        }

        spin_lock_irq(&desc->iuspin);

        /* re-verify data availability after taking the lock */
        if (desc->length == 0) {
                spin_unlock_irq(&desc->iuspin);
                kfree(kbuf);
                goto again;
        }

        if (cntr > desc->length)
                cntr = desc->length;

        memcpy(kbuf, desc->ubuf, cntr);

        for (i = 0; i < desc->length - cntr; i++)
                desc->ubuf[i] = desc->ubuf[i + cntr];

        desc->length -= cntr;
        /* in case we had outstanding data */
        if (!desc->length) {
                clear_bit(WDM_READ, &desc->flags);
                service_outstanding_interrupt(desc);
        }

        spin_unlock_irq(&desc->iuspin);

        if (copy_to_user(buffer, kbuf, cntr)) {
                rv = -EFAULT;
        } else {
                rv = cntr;
        }

        kfree(kbuf);

err:
        mutex_unlock(&desc->rlock);
        return rv;
}


It is not possible for me to reproduce the possible issue, nor test the
solution, due to lack of necessary hardware.
Could someone confirm my understanding and if necessary take care?

Thanks in advance and
Best Regards,
Francesco

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 220981] Potential data race in drivers/usb/class/cdc-wdm.c::wdm_read

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=220981

Oliver Neukum (oliver@neukum.org) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |oliver@neukum.org

--- Comment #1 from Oliver Neukum (oliver@neukum.org) ---
(In reply to Francesco Vincenti from comment #0)

> By looking at wdm_read() implementation:
> wdm_read() copies data from desc->ubuf to userspace through copy_to_user(),
> without holding the spin-lock desc->iuspin,
> while the RX completion handler wdm_in_callback() may simultaneously write
> desc->ubuf under the same spin-lock.

wdm_in_callback() adds only to the tail of the buffer, while wdm_read() only
takes from the head of the buffer while having dropped the lock. So, yes, there
can be simultaneous uses of the buffer, but they cannot corrupt data.

AFAICT this bug does not exist.

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 220981] Potential data race in drivers/usb/class/cdc-wdm.c::wdm_read

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=220981

--- Comment #2 from Francesco Vincenti (franci.vi@tiscali.it) ---
Hello Oliver,
thanks for your fast answer. Looking better at the code, the presence of
identical spin-lock in both contexts, producer-only-appends discipline,
re-verification of desc->length under the lock prevents data corruption.

Best Regards,
Francesco

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 220981] Potential data race in drivers/usb/class/cdc-wdm.c::wdm_read

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=220981

Francesco Vincenti (franci.vi@tiscali.it) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |ANSWERED

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.