Stable Archive on lore.kernel.org
 help / color / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, Alan Stern <stern@rowland.harvard.edu>,
	Christian Lamparter <chunkeey@gmail.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	syzbot+200d4bb11b23d929335f@syzkaller.appspotmail.com
Subject: [PATCH 5.2 27/61] p54usb: Fix race between disconnect and firmware loading
Date: Fri, 12 Jul 2019 14:19:40 +0200
Message-ID: <20190712121622.086164187@linuxfoundation.org> (raw)
In-Reply-To: <20190712121620.632595223@linuxfoundation.org>

From: Alan Stern <stern@rowland.harvard.edu>

commit 6e41e2257f1094acc37618bf6c856115374c6922 upstream.

The syzbot fuzzer found a bug in the p54 USB wireless driver.  The
issue involves a race between disconnect and the firmware-loader
callback routine, and it has several aspects.

One big problem is that when the firmware can't be loaded, the
callback routine tries to unbind the driver from the USB _device_ (by
calling device_release_driver) instead of from the USB _interface_ to
which it is actually bound (by calling usb_driver_release_interface).

The race involves access to the private data structure.  The driver's
disconnect handler waits for a completion that is signalled by the
firmware-loader callback routine.  As soon as the completion is
signalled, you have to assume that the private data structure may have
been deallocated by the disconnect handler -- even if the firmware was
loaded without errors.  However, the callback routine does access the
private data several times after that point.

Another problem is that, in order to ensure that the USB device
structure hasn't been freed when the callback routine runs, the driver
takes a reference to it.  This isn't good enough any more, because now
that the callback routine calls usb_driver_release_interface, it has
to ensure that the interface structure hasn't been freed.

Finally, the driver takes an unnecessary reference to the USB device
structure in the probe function and drops the reference in the
disconnect handler.  This extra reference doesn't accomplish anything,
because the USB core already guarantees that a device structure won't
be deallocated while a driver is still bound to any of its interfaces.

To fix these problems, this patch makes the following changes:

	Call usb_driver_release_interface() rather than
	device_release_driver().

	Don't signal the completion until after the important
	information has been copied out of the private data structure,
	and don't refer to the private data at all thereafter.

	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).

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-and-tested-by: syzbot+200d4bb11b23d929335f@syzkaller.appspotmail.com
CC: <stable@vger.kernel.org>
Acked-by: Christian Lamparter <chunkeey@gmail.com>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 drivers/net/wireless/intersil/p54/p54usb.c |   43 ++++++++++++-----------------
 1 file changed, 18 insertions(+), 25 deletions(-)

--- a/drivers/net/wireless/intersil/p54/p54usb.c
+++ b/drivers/net/wireless/intersil/p54/p54usb.c
@@ -30,6 +30,8 @@ MODULE_ALIAS("prism54usb");
 MODULE_FIRMWARE("isl3886usb");
 MODULE_FIRMWARE("isl3887usb");
 
+static struct usb_driver p54u_driver;
+
 /*
  * Note:
  *
@@ -918,9 +920,9 @@ static void p54u_load_firmware_cb(const
 {
 	struct p54u_priv *priv = context;
 	struct usb_device *udev = priv->udev;
+	struct usb_interface *intf = priv->intf;
 	int err;
 
-	complete(&priv->fw_wait_load);
 	if (firmware) {
 		priv->fw = firmware;
 		err = p54u_start_ops(priv);
@@ -929,26 +931,22 @@ static void p54u_load_firmware_cb(const
 		dev_err(&udev->dev, "Firmware not found.\n");
 	}
 
-	if (err) {
-		struct device *parent = priv->udev->dev.parent;
-
-		dev_err(&udev->dev, "failed to initialize device (%d)\n", err);
-
-		if (parent)
-			device_lock(parent);
+	complete(&priv->fw_wait_load);
+	/*
+	 * At this point p54u_disconnect may have already freed
+	 * the "priv" context. Do not use it anymore!
+	 */
+	priv = NULL;
 
-		device_release_driver(&udev->dev);
-		/*
-		 * At this point p54u_disconnect has already freed
-		 * the "priv" context. Do not use it anymore!
-		 */
-		priv = NULL;
+	if (err) {
+		dev_err(&intf->dev, "failed to initialize device (%d)\n", err);
 
-		if (parent)
-			device_unlock(parent);
+		usb_lock_device(udev);
+		usb_driver_release_interface(&p54u_driver, intf);
+		usb_unlock_device(udev);
 	}
 
-	usb_put_dev(udev);
+	usb_put_intf(intf);
 }
 
 static int p54u_load_firmware(struct ieee80211_hw *dev,
@@ -969,14 +967,14 @@ static int p54u_load_firmware(struct iee
 	dev_info(&priv->udev->dev, "Loading firmware file %s\n",
 	       p54u_fwlist[i].fw);
 
-	usb_get_dev(udev);
+	usb_get_intf(intf);
 	err = request_firmware_nowait(THIS_MODULE, 1, p54u_fwlist[i].fw,
 				      device, GFP_KERNEL, priv,
 				      p54u_load_firmware_cb);
 	if (err) {
 		dev_err(&priv->udev->dev, "(p54usb) cannot load firmware %s "
 					  "(%d)!\n", p54u_fwlist[i].fw, err);
-		usb_put_dev(udev);
+		usb_put_intf(intf);
 	}
 
 	return err;
@@ -1008,8 +1006,6 @@ static int p54u_probe(struct usb_interfa
 	skb_queue_head_init(&priv->rx_queue);
 	init_usb_anchor(&priv->submitted);
 
-	usb_get_dev(udev);
-
 	/* really lazy and simple way of figuring out if we're a 3887 */
 	/* TODO: should just stick the identification in the device table */
 	i = intf->altsetting->desc.bNumEndpoints;
@@ -1050,10 +1046,8 @@ static int p54u_probe(struct usb_interfa
 		priv->upload_fw = p54u_upload_firmware_net2280;
 	}
 	err = p54u_load_firmware(dev, intf);
-	if (err) {
-		usb_put_dev(udev);
+	if (err)
 		p54_free_common(dev);
-	}
 	return err;
 }
 
@@ -1069,7 +1063,6 @@ static void p54u_disconnect(struct usb_i
 	wait_for_completion(&priv->fw_wait_load);
 	p54_unregister_common(dev);
 
-	usb_put_dev(interface_to_usbdev(intf));
 	release_firmware(priv->fw);
 	p54_free_common(dev);
 }



  parent reply index

Thread overview: 80+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-12 12:19 [PATCH 5.2 00/61] 5.2.1-stable review Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 01/61] crypto: talitos - fix hash on SEC1 Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 02/61] crypto: lrw - use correct alignmask Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 03/61] crypto: talitos - rename alternative AEAD algos Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 04/61] fscrypt: dont set policy for a dead directory Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 05/61] udf: Fix incorrect final NOT_ALLOCATED (hole) extent length Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 06/61] media: stv0297: fix frequency range limit Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 07/61] ALSA: usb-audio: Fix parse of UAC2 Extension Units Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 08/61] ALSA: hda/realtek - Headphone Mic cant record after S3 Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 09/61] tpm: Actually fail on TPM errors during "get random" Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 10/61] tpm: Fix TPM 1.2 Shutdown sequence to prevent future TPM operations Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 11/61] block: fix .bi_size overflow Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 12/61] block, bfq: NULL out the bic when its no longer valid Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 13/61] perf intel-pt: Fix itrace defaults for perf script Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 14/61] perf auxtrace: " Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 15/61] perf intel-pt: Fix itrace defaults for perf script intel-pt documentation Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 16/61] perf pmu: Fix uncore PMU alias list for ARM64 Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 17/61] perf thread-stack: Fix thread stack return from kernel for kernel-only case Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 18/61] perf header: Assign proper ff->ph in perf_event__synthesize_features() Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 19/61] x86/ptrace: Fix possible spectre-v1 in ptrace_get_debugreg() Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 20/61] x86/tls: Fix possible spectre-v1 in do_get_thread_area() Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 21/61] Documentation: Add section about CPU vulnerabilities for Spectre Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 22/61] Documentation/admin: Remove the vsyscall=native documentation Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 23/61] mwifiex: Dont abort on small, spec-compliant vendor IEs Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 24/61] USB: serial: ftdi_sio: add ID for isodebug v1 Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 25/61] USB: serial: option: add support for GosunCn ME3630 RNDIS mode Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 26/61] Revert "serial: 8250: Dont service RX FIFO if interrupts are disabled" Greg Kroah-Hartman
2019-07-12 12:19 ` Greg Kroah-Hartman [this message]
2019-07-12 12:19 ` [PATCH 5.2 28/61] usb: gadget: f_fs: data_len used before properly set Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 29/61] usb: gadget: ether: Fix race between gether_disconnect and rx_submit Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 30/61] usb: dwc2: use a longer AHB idle timeout in dwc2_core_reset() Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 31/61] usb: renesas_usbhs: add a workaround for a race condition of workqueue Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 32/61] drivers/usb/typec/tps6598x.c: fix portinfo width Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 33/61] drivers/usb/typec/tps6598x.c: fix 4CC cmd write Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 34/61] p54: fix crash during initialization Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 35/61] staging: comedi: dt282x: fix a null pointer deref on interrupt Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 36/61] staging: wilc1000: fix error path cleanup in wilc_wlan_initialize() Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 37/61] staging: bcm2835-camera: Restore return behavior of ctrl_set_bitrate() Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 38/61] staging: comedi: amplc_pci230: fix null pointer deref on interrupt Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 39/61] staging: mt7621-pci: fix PCIE_FTS_NUM_LO macro Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 40/61] HID: Add another Primax PIXART OEM mouse quirk Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 41/61] lkdtm: support llvm-objcopy Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 42/61] binder: fix memory leak in error path Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 43/61] binder: return errors from buffer copy functions Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 44/61] iio: adc: stm32-adc: add missing vdda-supply Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 45/61] coresight: Potential uninitialized variable in probe() Greg Kroah-Hartman
2019-07-12 12:19 ` [PATCH 5.2 46/61] coresight: etb10: Do not call smp_processor_id from preemptible Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 47/61] coresight: tmc-etr: Do not call smp_processor_id() " Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 48/61] coresight: tmc-etr: alloc_perf_buf: Do not call smp_processor_id " Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 49/61] coresight: tmc-etf: " Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 50/61] carl9170: fix misuse of device driver API Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 51/61] Revert "x86/build: Move _etext to actual end of .text" Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 52/61] VMCI: Fix integer overflow in VMCI handle arrays Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 53/61] staging: vchiq_2835_arm: revert "quit using custom down_interruptible()" Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 54/61] staging: vchiq: make wait events interruptible Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 55/61] staging: vchiq: revert "switch to wait_for_completion_killable" Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 56/61] staging: fsl-dpaa2/ethsw: fix memory leak of switchdev_work Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 57/61] staging: bcm2835-camera: Replace spinlock protecting context_map with mutex Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 58/61] staging: bcm2835-camera: Ensure all buffers are returned on disable Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 59/61] staging: bcm2835-camera: Remove check of the number of buffers supplied Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 60/61] staging: bcm2835-camera: Handle empty EOS buffers whilst streaming Greg Kroah-Hartman
2019-07-12 12:20 ` [PATCH 5.2 61/61] staging: rtl8712: reduce stack usage, again Greg Kroah-Hartman
2019-07-12 13:36 ` [PATCH 5.2 00/61] 5.2.1-stable review Jon Hunter
2019-07-12 15:31   ` Greg Kroah-Hartman
2019-07-12 16:10     ` Jon Hunter
2019-07-12 16:51       ` Greg Kroah-Hartman
2019-07-12 17:00 ` Major Hayden
2019-07-12 17:17   ` Dan Rue
2019-07-12 22:07 ` shuah
2019-07-13  8:23   ` Greg Kroah-Hartman
2019-07-13  3:04 ` Naresh Kamboju
2019-07-13 14:32   ` Greg Kroah-Hartman
2019-07-13 10:15 ` Shreeya Patel
2019-07-13 15:21   ` Greg Kroah-Hartman
2019-07-13 20:37 ` Luke Nowakowski-Krijger
2019-07-14  6:02   ` Greg Kroah-Hartman
2019-07-13 22:04 ` Guenter Roeck
2019-07-14  6:02   ` Greg Kroah-Hartman
2019-07-14  5:35 ` Kelsey Skunberg
2019-07-14  6:02   ` Greg Kroah-Hartman

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190712121622.086164187@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=chunkeey@gmail.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    --cc=syzbot+200d4bb11b23d929335f@syzkaller.appspotmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Stable Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/stable/0 stable/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 stable stable/ https://lore.kernel.org/stable \
		stable@vger.kernel.org stable@archiver.kernel.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.stable


AGPL code for this site: git clone https://public-inbox.org/ public-inbox