usb: xhci-pci: reorder removal to avoid use-after-free

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

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

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

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