Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] usb: xhci-pci: reorder removal to avoid use-after-free
@ 2019-08-14 11:39 Schmid, Carsten
  2019-08-14 12:54 ` Hans de Goede
  0 siblings, 1 reply; 6+ messages in thread
From: Schmid, Carsten @ 2019-08-14 11:39 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-usb, linux-kernel, linux-pci

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 resorce 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>
---
 drivers/usb/host/xhci-ext-caps.c | 22 ++++++++++++----------
 drivers/usb/host/xhci-pci.c      |  4 ++++
 drivers/usb/host/xhci-pci.h      | 19 +++++++++++++++++++
 drivers/usb/host/xhci.h          |  1 +
 4 files changed, 36 insertions(+), 10 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..d2ab1e2a39c0 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->pdev = pdev;
 
 	return 0;
 }
@@ -88,3 +82,11 @@ 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->pdev)
+		platform_device_unregister(priv->pdev);
+}
+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..a4d094df56f7 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,
 };
 
@@ -393,6 +395,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..ead9618d7368
--- /dev/null
+++ b/drivers/usb/host/xhci-pci.h
@@ -0,0 +1,19 @@
+/*
+ * 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 *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] 6+ messages in thread

* Re: [PATCH] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-14 11:39 [PATCH] usb: xhci-pci: reorder removal to avoid use-after-free Schmid, Carsten
@ 2019-08-14 12:54 ` Hans de Goede
  2019-08-14 13:32   ` AW: " Schmid, Carsten
  0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2019-08-14 12:54 UTC (permalink / raw)
  To: Schmid, Carsten; +Cc: linux-usb, linux-kernel, linux-pci

Hi,

On 14-08-19 13:39, 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 resorce 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>

Assuming this has been tested, overal this looks good to me.

But there are 2 things to fix:

1) Maybe pick a more descriptive struct member name then pdev.
    pdev with pci-devices often points to a pci_device ...
    How about: role_switch_pdev ?

2) xhci_ext_cap_init() is not the last call which can fail in
    xhci_pci_probe(), since you now no longer use devm_add_action_or_reset
    for auto-cleanup, you must now manually cleanup by calling
    xhci_ext_cap_remove() when later steps of xhci_pci_probe() fail.
    it looks like you will need a new ext_cap_remove error-exit label
    for this put above the put_usb3_hcd label and goto this new label
    instead of to put_usb3_hcd in all error paths after a successful call
    to xhci_ext_cap_init()

Regards,

Hans


> ---
>   drivers/usb/host/xhci-ext-caps.c | 22 ++++++++++++----------
>   drivers/usb/host/xhci-pci.c      |  4 ++++
>   drivers/usb/host/xhci-pci.h      | 19 +++++++++++++++++++
>   drivers/usb/host/xhci.h          |  1 +
>   4 files changed, 36 insertions(+), 10 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..d2ab1e2a39c0 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->pdev = pdev;
>   
>   	return 0;
>   }
> @@ -88,3 +82,11 @@ 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->pdev)
> +		platform_device_unregister(priv->pdev);
> +}
> +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..a4d094df56f7 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,
>   };
>   
> @@ -393,6 +395,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..ead9618d7368
> --- /dev/null
> +++ b/drivers/usb/host/xhci-pci.h
> @@ -0,0 +1,19 @@
> +/*
> + * 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 *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);
> 

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

* AW: [PATCH] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-14 12:54 ` Hans de Goede
@ 2019-08-14 13:32   ` " Schmid, Carsten
  2019-08-14 13:36     ` Hans de Goede
  0 siblings, 1 reply; 6+ messages in thread
From: Schmid, Carsten @ 2019-08-14 13:32 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-usb, linux-kernel, linux-pci

> > 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 resorce 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>
> 
> Assuming this has been tested, overal this looks good to me.

Tested on 4.14.129, ported to v5.2.7, compiled there.

> 
> But there are 2 things to fix:
> 
> 1) Maybe pick a more descriptive struct member name then pdev.
>     pdev with pci-devices often points to a pci_device ...
>     How about: role_switch_pdev ?

Ok, good point. Had platform dev pdev in mind ...

> 
> 2) xhci_ext_cap_init() is not the last call which can fail in
>     xhci_pci_probe(), since you now no longer use
> devm_add_action_or_reset
>     for auto-cleanup, you must now manually cleanup by calling
>     xhci_ext_cap_remove() when later steps of xhci_pci_probe() fail.
>     it looks like you will need a new ext_cap_remove error-exit label
>     for this put above the put_usb3_hcd label and goto this new label
>     instead of to put_usb3_hcd in all error paths after a successful call
>     to xhci_ext_cap_init()

Right. Will review this path and correct accordingly.

Maybe an additional label isn't required because pdev is only set when
xhci_ext_cap_init created the platform device, and xhci_ext_cap_remove
checks for pdev being set.
So a call to xhci_ext_cap_remove doesn't harm if pdev is not set up yet.
But for readability it might be better to create a label.

Best regards
Carsten

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

* Re: AW: [PATCH] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-14 13:32   ` AW: " Schmid, Carsten
@ 2019-08-14 13:36     ` Hans de Goede
  2019-08-14 14:32       ` [PATCH v2] " Schmid, Carsten
  0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2019-08-14 13:36 UTC (permalink / raw)
  To: Schmid, Carsten; +Cc: linux-usb, linux-kernel, linux-pci

Hi,

On 14-08-19 15:32, 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 resorce 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>
>>
>> Assuming this has been tested, overal this looks good to me.
> 
> Tested on 4.14.129, ported to v5.2.7, compiled there.
> 
>>
>> But there are 2 things to fix:
>>
>> 1) Maybe pick a more descriptive struct member name then pdev.
>>      pdev with pci-devices often points to a pci_device ...
>>      How about: role_switch_pdev ?
> 
> Ok, good point. Had platform dev pdev in mind ...
> 
>>
>> 2) xhci_ext_cap_init() is not the last call which can fail in
>>      xhci_pci_probe(), since you now no longer use
>> devm_add_action_or_reset
>>      for auto-cleanup, you must now manually cleanup by calling
>>      xhci_ext_cap_remove() when later steps of xhci_pci_probe() fail.
>>      it looks like you will need a new ext_cap_remove error-exit label
>>      for this put above the put_usb3_hcd label and goto this new label
>>      instead of to put_usb3_hcd in all error paths after a successful call
>>      to xhci_ext_cap_init()
> 
> Right. Will review this path and correct accordingly.
> 
> Maybe an additional label isn't required because pdev is only set when
> xhci_ext_cap_init created the platform device, and xhci_ext_cap_remove
> checks for pdev being set.
> So a call to xhci_ext_cap_remove doesn't harm if pdev is not set up yet.
> But for readability it might be better to create a label.

Right, when taking a quick look myself I realized that an extra label would
not be necessary, but not having the extra label will confuse the reader
of the code, since now we are undoing something which we did not do,
so I would prefer if you use the extra label.

Regards,

Hans


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

* [PATCH v2] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-14 13:36     ` Hans de Goede
@ 2019-08-14 14:32       ` " Schmid, Carsten
  2019-08-15 15:27         ` Hans de Goede
  0 siblings, 1 reply; 6+ messages in thread
From: Schmid, Carsten @ 2019-08-14 14:32 UTC (permalink / raw)
  To: Hans de Goede; +Cc: linux-usb, linux-kernel, linux-pci

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 resorce 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>
---
v2:
  - more speaking name for private data element
  - consider failure in driver init sequence
  - fix minor issues found by checkpatch.pl
---
 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] 6+ messages in thread

* Re: [PATCH v2] usb: xhci-pci: reorder removal to avoid use-after-free
  2019-08-14 14:32       ` [PATCH v2] " Schmid, Carsten
@ 2019-08-15 15:27         ` Hans de Goede
  0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2019-08-15 15:27 UTC (permalink / raw)
  To: Schmid, Carsten; +Cc: linux-usb, linux-kernel, linux-pci

Hi,

On 14-08-19 16:32, 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 resorce 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>

Patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> ---
> v2:
>    - more speaking name for private data element
>    - consider failure in driver init sequence
>    - fix minor issues found by checkpatch.pl
> ---
>   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);
> 

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

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 11:39 [PATCH] usb: xhci-pci: reorder removal to avoid use-after-free Schmid, Carsten
2019-08-14 12:54 ` Hans de Goede
2019-08-14 13:32   ` AW: " Schmid, Carsten
2019-08-14 13:36     ` Hans de Goede
2019-08-14 14:32       ` [PATCH v2] " Schmid, Carsten
2019-08-15 15:27         ` 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


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