Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
@ 2019-08-16  9:03 Schmid, Carsten
  2019-08-22 15:23 ` Mathias Nyman
  0 siblings, 1 reply; 5+ messages in thread
From: Schmid, Carsten @ 2019-08-16  9:03 UTC (permalink / raw)
  To: Mathias Nyman; +Cc: Hans de Goede, linux-usb

On driver removal, the platform_device_unregister call
attached through devm_add_action_or_reset was executed
after usb_hcd_pci_remove.
This lead to a use-after-free for the iomem resource of
the xhci-ext-caps driver in the platform removal
because the parent of the resource was freed earlier.

Fix this by reordering of the removal sequence.

Signed-off-by: Carsten Schmid <carsten_schmid@mentor.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Carsten Schmid <carsten_schmid@mentor.com>
---
v2:
   - more speaking name for private data element
   - consider failure in driver init sequence
   - fix minor issues found by checkpatch.pl
v3:
   - corrected typo: resorce -> resource
   - added Reviewed by and Tested-by
Resend v3:
   - made a mistake when copying patch content to mail in v3
---
 drivers/usb/host/xhci-ext-caps.c | 25 +++++++++++++++----------
 drivers/usb/host/xhci-pci.c      |  8 +++++++-
 drivers/usb/host/xhci-pci.h      | 20 ++++++++++++++++++++
 drivers/usb/host/xhci.h          |  1 +
 4 files changed, 43 insertions(+), 11 deletions(-)
 create mode 100644 drivers/usb/host/xhci-pci.h

diff --git a/drivers/usb/host/xhci-ext-caps.c b/drivers/usb/host/xhci-ext-caps.c
index 399113f9fc5c..28a7d53ecf2c 100644
--- a/drivers/usb/host/xhci-ext-caps.c
+++ b/drivers/usb/host/xhci-ext-caps.c
@@ -7,21 +7,19 @@
 
 #include <linux/platform_device.h>
 #include "xhci.h"
+#include "xhci-pci.h"
 
 #define USB_SW_DRV_NAME		"intel_xhci_usb_sw"
 #define USB_SW_RESOURCE_SIZE	0x400
 
-static void xhci_intel_unregister_pdev(void *arg)
-{
-	platform_device_unregister(arg);
-}
-
 static int xhci_create_intel_xhci_sw_pdev(struct xhci_hcd *xhci, u32 cap_offset)
 {
 	struct usb_hcd *hcd = xhci_to_hcd(xhci);
 	struct device *dev = hcd->self.controller;
 	struct platform_device *pdev;
 	struct resource	res = { 0, };
+	struct xhci_pci_priv *priv = (struct xhci_pci_priv *)xhci->priv;
+
 	int ret;
 
 	pdev = platform_device_alloc(USB_SW_DRV_NAME, PLATFORM_DEVID_NONE);
@@ -52,11 +50,7 @@ static int xhci_create_intel_xhci_sw_pdev(struct xhci_hcd *xhci, u32 cap_offset)
 		return ret;
 	}
 
-	ret = devm_add_action_or_reset(dev, xhci_intel_unregister_pdev, pdev);
-	if (ret) {
-		dev_err(dev, "couldn't add unregister action for intel_xhci_usb_sw pdev\n");
-		return ret;
-	}
+	priv->role_switch_pdev = pdev;
 
 	return 0;
 }
@@ -88,3 +82,14 @@ int xhci_ext_cap_init(struct xhci_hcd *xhci)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(xhci_ext_cap_init);
+
+void xhci_ext_cap_remove(struct xhci_hcd *xhci)
+{
+	struct xhci_pci_priv *priv = (struct xhci_pci_priv *)xhci->priv;
+
+	if (priv->role_switch_pdev) {
+		platform_device_unregister(priv->role_switch_pdev);
+		priv->role_switch_pdev = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(xhci_ext_cap_remove);
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index c2fe218e051f..f2201f380c17 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -14,6 +14,7 @@
 #include <linux/acpi.h>
 
 #include "xhci.h"
+#include "xhci-pci.h"
 #include "xhci-trace.h"
 
 #define SSIC_PORT_NUM		2
@@ -62,6 +63,7 @@ static struct hc_driver __read_mostly xhci_pci_hc_driver;
 static int xhci_pci_setup(struct usb_hcd *hcd);
 
 static const struct xhci_driver_overrides xhci_pci_overrides __initconst = {
+	.extra_priv_size = sizeof(struct xhci_pci_priv),
 	.reset = xhci_pci_setup,
 };
 
@@ -350,7 +352,7 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	retval = usb_add_hcd(xhci->shared_hcd, dev->irq,
 			IRQF_SHARED);
 	if (retval)
-		goto put_usb3_hcd;
+		goto remove_ext_cap;
 	/* Roothub already marked as USB 3.0 speed */
 
 	if (!(xhci->quirks & XHCI_BROKEN_STREAMS) &&
@@ -368,6 +370,8 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	return 0;
 
+remove_ext_cap:
+	xhci_ext_cap_remove(xhci);
 put_usb3_hcd:
 	usb_put_hcd(xhci->shared_hcd);
 dealloc_usb2_hcd:
@@ -393,6 +397,8 @@ static void xhci_pci_remove(struct pci_dev *dev)
 		xhci->shared_hcd = NULL;
 	}
 
+	xhci_ext_cap_remove(xhci);
+
 	/* Workaround for spurious wakeups at shutdown with HSW */
 	if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
 		pci_set_power_state(dev, PCI_D3hot);
diff --git a/drivers/usb/host/xhci-pci.h b/drivers/usb/host/xhci-pci.h
new file mode 100644
index 000000000000..fc0cde231679
--- /dev/null
+++ b/drivers/usb/host/xhci-pci.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * xhci-pci.h - xHCI extended capability handling platform Glue.
+ *
+ * Copyright (C) 2019 Mentor Graphics (Deutschland) GmbH
+ * Derived from xhci-plat.h
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#ifndef _XHCI_PCI_H
+#define _XHCI_PCI_H
+
+struct xhci_pci_priv {
+	struct platform_device *role_switch_pdev;
+};
+
+#endif	/* _XHCI_PCI_H */
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index fabbce1c542a..847d2021fc2c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2052,6 +2052,7 @@ void xhci_init_driver(struct hc_driver *drv,
 		      const struct xhci_driver_overrides *over);
 int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
 int xhci_ext_cap_init(struct xhci_hcd *xhci);
+void xhci_ext_cap_remove(struct xhci_hcd *xhci);
 
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
 int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
-- 
2.17.1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-16  9:03 [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free Schmid, Carsten
@ 2019-08-22 15:23 ` Mathias Nyman
  2019-08-22 15:25   ` Hans de Goede
  0 siblings, 1 reply; 5+ messages in thread
From: Mathias Nyman @ 2019-08-22 15:23 UTC (permalink / raw)
  To: Schmid, Carsten; +Cc: Hans de Goede, linux-usb

On 16.8.2019 12.03, Schmid, Carsten wrote:
> On driver removal, the platform_device_unregister call
> attached through devm_add_action_or_reset was executed
> after usb_hcd_pci_remove.
> This lead to a use-after-free for the iomem resource of
> the xhci-ext-caps driver in the platform removal
> because the parent of the resource was freed earlier.
> 
> Fix this by reordering of the removal sequence.
> 

Could all this be avoided if usb_hcd_pci_probe()
used managed device resources as well?
(using devm_request_mem_region(), and devm_ioremap_nocache())

This way the iomem resource would be added to the same devres list
as the platform_unregister_call, and the iomem resource should be
released after the platform_device_unregister as devres_release_all()
releases the resources in reverse order.

-Mathias

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-22 15:23 ` Mathias Nyman
@ 2019-08-22 15:25   ` Hans de Goede
  2019-08-22 15:48     ` AW: " Schmid, Carsten
  0 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2019-08-22 15:25 UTC (permalink / raw)
  To: Mathias Nyman, Schmid, Carsten; +Cc: linux-usb

Hi,

On 22-08-19 17:23, Mathias Nyman wrote:
> On 16.8.2019 12.03, Schmid, Carsten wrote:
>> On driver removal, the platform_device_unregister call
>> attached through devm_add_action_or_reset was executed
>> after usb_hcd_pci_remove.
>> This lead to a use-after-free for the iomem resource of
>> the xhci-ext-caps driver in the platform removal
>> because the parent of the resource was freed earlier.
>>
>> Fix this by reordering of the removal sequence.
>>
> 
> Could all this be avoided if usb_hcd_pci_probe()
> used managed device resources as well?
> (using devm_request_mem_region(), and devm_ioremap_nocache())
> 
> This way the iomem resource would be added to the same devres list
> as the platform_unregister_call, and the iomem resource should be
> released after the platform_device_unregister as devres_release_all()
> releases the resources in reverse order.

Yes I believe that that would work.

Regards,

Hans


^ permalink raw reply	[flat|nested] 5+ messages in thread

* AW: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-22 15:25   ` Hans de Goede
@ 2019-08-22 15:48     ` " Schmid, Carsten
  2019-08-22 20:14       ` Hans de Goede
  0 siblings, 1 reply; 5+ messages in thread
From: Schmid, Carsten @ 2019-08-22 15:48 UTC (permalink / raw)
  To: Hans de Goede, Mathias Nyman; +Cc: linux-usb

> On 22-08-19 17:23, Mathias Nyman wrote:
> > On 16.8.2019 12.03, Schmid, Carsten wrote:
> >> On driver removal, the platform_device_unregister call
> >> attached through devm_add_action_or_reset was executed
> >> after usb_hcd_pci_remove.
> >> This lead to a use-after-free for the iomem resource of
> >> the xhci-ext-caps driver in the platform removal
> >> because the parent of the resource was freed earlier.
> >>
> >> Fix this by reordering of the removal sequence.
> >>
> >
> > Could all this be avoided if usb_hcd_pci_probe()
> > used managed device resources as well?
> > (using devm_request_mem_region(), and devm_ioremap_nocache())
> >
> > This way the iomem resource would be added to the same devres list
> > as the platform_unregister_call, and the iomem resource should be
> > released after the platform_device_unregister as devres_release_all()
> > releases the resources in reverse order.
> 
> Yes I believe that that would work.
> 
I don't think so, because xhci_create_intel_xhci_sw_pdev registers it's 
resource through platform_device_add_resources which does not use devm_.

The only thing what is done through devm in xhci_create_intel_xhci_sw_pdev is
ret = devm_add_action_or_reset(...)

Carsten

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: AW: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-22 15:48     ` AW: " Schmid, Carsten
@ 2019-08-22 20:14       ` Hans de Goede
  0 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2019-08-22 20:14 UTC (permalink / raw)
  To: Schmid, Carsten, Mathias Nyman; +Cc: linux-usb

Hi,

On 22-08-19 17:48, Schmid, Carsten wrote:
>> On 22-08-19 17:23, Mathias Nyman wrote:
>>> On 16.8.2019 12.03, Schmid, Carsten wrote:
>>>> On driver removal, the platform_device_unregister call
>>>> attached through devm_add_action_or_reset was executed
>>>> after usb_hcd_pci_remove.
>>>> This lead to a use-after-free for the iomem resource of
>>>> the xhci-ext-caps driver in the platform removal
>>>> because the parent of the resource was freed earlier.
>>>>
>>>> Fix this by reordering of the removal sequence.
>>>>
>>>
>>> Could all this be avoided if usb_hcd_pci_probe()
>>> used managed device resources as well?
>>> (using devm_request_mem_region(), and devm_ioremap_nocache())
>>>
>>> This way the iomem resource would be added to the same devres list
>>> as the platform_unregister_call, and the iomem resource should be
>>> released after the platform_device_unregister as devres_release_all()
>>> releases the resources in reverse order.
>>
>> Yes I believe that that would work.
>>
> I don't think so, because xhci_create_intel_xhci_sw_pdev registers it's
> resource through platform_device_add_resources which does not use devm_.
> 
> The only thing what is done through devm in xhci_create_intel_xhci_sw_pdev is
> ret = devm_add_action_or_reset(...)

Right, so if I understand correctly the problem with the current code (before your patch)
is:

Probe:
1. usb_hcd_pci_probe() uses request_mem_region()
2. xhci_create_intel_xhci_sw_pde uses platform_device_add_resources() which are a child of the previous region

Remove:
3. usb_hcd_pci_remove does release_region() while the child region is still in use
4. At end of remove() devm code calls xhci_intel_unregister_pdev

And the problem is we want to swap 3 and 4.

Now if we make usb_hcd_pci_probe() use devm_request_mem_region and drop the cleanup from usb_hcd_pci_remove

Probe:
1. usb_hcd_pci_probe() uses devm_request_mem_region(), this registers a release_region() with devm as cleanup
2. xhci_create_intel_xhci_sw_pde uses platform_device_add_resources() which are a child of the previous region and calls
    devm_add_action_or_reset() to add xhci_intel_unregister_pdev as cleanup

Remove:
3. usb_hcd_pci_remove does nothing of relevance
4. At end of remove() devm code runs and calls the cleanups in reverse order of registration! so it calls:
    xhci_intel_unregister_pdev()
    release_region()

Note the trick here is that the devm framework calls devm registered cleanup functions in reverse order of their registration, putting things in the right order.

Regards,

Hans

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-16  9:03 [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free Schmid, Carsten
2019-08-22 15:23 ` Mathias Nyman
2019-08-22 15:25   ` Hans de Goede
2019-08-22 15:48     ` AW: " Schmid, Carsten
2019-08-22 20:14       ` Hans de Goede

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/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 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


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