USB: cdc-wdm: fix race between write and disconnect due to flag abuse [Linux 4.19.70]

This Linux kernel change "USB: cdc-wdm: fix race between write and disconnect due to flag abuse" is included in the Linux 4.19.70 release. This change is authored by Oliver Neukum <oneukum [at] suse.com> on Tue Aug 27 12:34:36 2019 +0200. The commit for this change in Linux stable tree is ebad9fd (patch) which is from upstream commit 1426bd2. The same Linux upstream change may have been applied to various maintained Linux releases and you can find all Linux releases containing changes from upstream 1426bd2.

USB: cdc-wdm: fix race between write and disconnect due to flag abuse

commit 1426bd2c9f7e3126e2678e7469dca9fd9fc6dd3e upstream.

In case of a disconnect an ongoing flush() has to be made fail.
Nevertheless we cannot be sure that any pending URB has already
finished, so although they will never succeed, they still must
not be touched.
The clean solution for this is to check for WDM_IN_USE
and WDM_DISCONNECTED in flush(). There is no point in ever
clearing WDM_IN_USE, as no further writes make sense.

The issue is as old as the driver.

Fixes: afba937e540c9 ("USB: CDC WDM driver")
Reported-by: syzbot+d232cca6ec42c2edb3fc@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@suse.com>
Cc: stable <stable@vger.kernel.org>
Link: https://lore.kernel.org/r/20190827103436.21143-1-oneukum@suse.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

There are 16 lines of Linux source code added/deleted in this change. Code changes to Linux kernel are as follows.

 drivers/usb/class/cdc-wdm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index bec581f..b8a1fde 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -587,10 +587,20 @@ static int wdm_flush(struct file *file, fl_owner_t id)
 {
    struct wdm_device *desc = file->private_data;

-   wait_event(desc->wait, !test_bit(WDM_IN_USE, &desc->flags));
+   wait_event(desc->wait,
+           /*
+            * needs both flags. We cannot do with one
+            * because resetting it would cause a race
+            * with write() yet we need to signal
+            * a disconnect
+            */
+           !test_bit(WDM_IN_USE, &desc->flags) ||
+           test_bit(WDM_DISCONNECTING, &desc->flags));

    /* cannot dereference desc->intf if WDM_DISCONNECTING */
-   if (desc->werr < 0 && !test_bit(WDM_DISCONNECTING, &desc->flags))
+   if (test_bit(WDM_DISCONNECTING, &desc->flags))
+       return -ENODEV;
+   if (desc->werr < 0)
        dev_err(&desc->intf->dev, "Error in flush path: %d\n",
            desc->werr);

@@ -974,8 +984,6 @@ static void wdm_disconnect(struct usb_interface *intf)
    spin_lock_irqsave(&desc->iuspin, flags);
    set_bit(WDM_DISCONNECTING, &desc->flags);
    set_bit(WDM_READ, &desc->flags);
-   /* to terminate pending flushes */
-   clear_bit(WDM_IN_USE, &desc->flags);
    spin_unlock_irqrestore(&desc->iuspin, flags);
    wake_up_all(&desc->wait);
    mutex_lock(&desc->rlock);

Leave a Reply

Your email address will not be published. Required fields are marked *