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

[bugzilla-daemon@kernel.org]

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.