Commit 9e0150f6 authored by Christian Lamparter's avatar Christian Lamparter Committed by Greg Kroah-Hartman

carl9170: fix misuse of device driver API

commit feb09b29 upstream.

This patch follows Alan Stern's recent patch:
"p54: Fix race between disconnect and firmware loading"

that overhauled carl9170 buggy firmware loading and driver
unbinding procedures.

Since the carl9170 code was adapted from p54 it uses the
same functions and is likely to have the same problem, but
it's just that the syzbot hasn't reproduce them (yet).

a summary from the changes (copied from the p54 patch):
 * Call usb_driver_release_interface() rather than
   device_release_driver().

 * Lock udev (the interface's parent) before unbinding the
   driver instead of locking udev->parent.

 * During the firmware loading process, take a reference
   to the USB interface instead of the USB device.

 * Don't take an unnecessary reference to the device during
   probe (and then don't drop it during disconnect).

and

 * Make sure to prevent use-after-free bugs by explicitly
   setting the driver context to NULL after signaling the
   completion.

Cc: <stable@vger.kernel.org>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: default avatarChristian Lamparter <chunkeey@gmail.com>
Acked-by: default avatarAlan Stern <stern@rowland.harvard.edu>
Signed-off-by: default avatarKalle Valo <kvalo@codeaurora.org>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent c1f5ded2
...@@ -128,6 +128,8 @@ static const struct usb_device_id carl9170_usb_ids[] = { ...@@ -128,6 +128,8 @@ static const struct usb_device_id carl9170_usb_ids[] = {
}; };
MODULE_DEVICE_TABLE(usb, carl9170_usb_ids); MODULE_DEVICE_TABLE(usb, carl9170_usb_ids);
static struct usb_driver carl9170_driver;
static void carl9170_usb_submit_data_urb(struct ar9170 *ar) static void carl9170_usb_submit_data_urb(struct ar9170 *ar)
{ {
struct urb *urb; struct urb *urb;
...@@ -966,32 +968,28 @@ static int carl9170_usb_init_device(struct ar9170 *ar) ...@@ -966,32 +968,28 @@ static int carl9170_usb_init_device(struct ar9170 *ar)
static void carl9170_usb_firmware_failed(struct ar9170 *ar) static void carl9170_usb_firmware_failed(struct ar9170 *ar)
{ {
struct device *parent = ar->udev->dev.parent; /* Store a copies of the usb_interface and usb_device pointer locally.
struct usb_device *udev; * This is because release_driver initiates carl9170_usb_disconnect,
* which in turn frees our driver context (ar).
/*
* Store a copy of the usb_device pointer locally.
* This is because device_release_driver initiates
* carl9170_usb_disconnect, which in turn frees our
* driver context (ar).
*/ */
udev = ar->udev; struct usb_interface *intf = ar->intf;
struct usb_device *udev = ar->udev;
complete(&ar->fw_load_wait); complete(&ar->fw_load_wait);
/* at this point 'ar' could be already freed. Don't use it anymore */
ar = NULL;
/* unbind anything failed */ /* unbind anything failed */
if (parent) usb_lock_device(udev);
device_lock(parent); usb_driver_release_interface(&carl9170_driver, intf);
usb_unlock_device(udev);
device_release_driver(&udev->dev);
if (parent)
device_unlock(parent);
usb_put_dev(udev); usb_put_intf(intf);
} }
static void carl9170_usb_firmware_finish(struct ar9170 *ar) static void carl9170_usb_firmware_finish(struct ar9170 *ar)
{ {
struct usb_interface *intf = ar->intf;
int err; int err;
err = carl9170_parse_firmware(ar); err = carl9170_parse_firmware(ar);
...@@ -1009,7 +1007,7 @@ static void carl9170_usb_firmware_finish(struct ar9170 *ar) ...@@ -1009,7 +1007,7 @@ static void carl9170_usb_firmware_finish(struct ar9170 *ar)
goto err_unrx; goto err_unrx;
complete(&ar->fw_load_wait); complete(&ar->fw_load_wait);
usb_put_dev(ar->udev); usb_put_intf(intf);
return; return;
err_unrx: err_unrx:
...@@ -1052,7 +1050,6 @@ static int carl9170_usb_probe(struct usb_interface *intf, ...@@ -1052,7 +1050,6 @@ static int carl9170_usb_probe(struct usb_interface *intf,
return PTR_ERR(ar); return PTR_ERR(ar);
udev = interface_to_usbdev(intf); udev = interface_to_usbdev(intf);
usb_get_dev(udev);
ar->udev = udev; ar->udev = udev;
ar->intf = intf; ar->intf = intf;
ar->features = id->driver_info; ar->features = id->driver_info;
...@@ -1094,15 +1091,14 @@ static int carl9170_usb_probe(struct usb_interface *intf, ...@@ -1094,15 +1091,14 @@ static int carl9170_usb_probe(struct usb_interface *intf,
atomic_set(&ar->rx_anch_urbs, 0); atomic_set(&ar->rx_anch_urbs, 0);
atomic_set(&ar->rx_pool_urbs, 0); atomic_set(&ar->rx_pool_urbs, 0);
usb_get_dev(ar->udev); usb_get_intf(intf);
carl9170_set_state(ar, CARL9170_STOPPED); carl9170_set_state(ar, CARL9170_STOPPED);
err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME, err = request_firmware_nowait(THIS_MODULE, 1, CARL9170FW_NAME,
&ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2); &ar->udev->dev, GFP_KERNEL, ar, carl9170_usb_firmware_step2);
if (err) { if (err) {
usb_put_dev(udev); usb_put_intf(intf);
usb_put_dev(udev);
carl9170_free(ar); carl9170_free(ar);
} }
return err; return err;
...@@ -1131,7 +1127,6 @@ static void carl9170_usb_disconnect(struct usb_interface *intf) ...@@ -1131,7 +1127,6 @@ static void carl9170_usb_disconnect(struct usb_interface *intf)
carl9170_release_firmware(ar); carl9170_release_firmware(ar);
carl9170_free(ar); carl9170_free(ar);
usb_put_dev(udev);
} }
#ifdef CONFIG_PM #ifdef CONFIG_PM
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment