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

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: [email protected]
          Reporter: [email protected]
        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.

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

Oliver Neukum ([email protected]) changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]

--- Comment #1 from Oliver Neukum ([email protected]) --- (In reply to Francesco Vincenti from comment #0)

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.

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

--- Comment #2 from Francesco Vincenti ([email protected]) --- 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.

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

Francesco Vincenti ([email protected]) 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.