Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: "Schmid, Carsten" <Carsten_Schmid@mentor.com>
Cc: "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [PATCH] usb: xhci-pci: reorder removal to avoid use-after-free
Date: Wed, 14 Aug 2019 14:54:31 +0200
Message-ID: <15aa45c7-6e45-d03f-9336-4291f8b2dc66@redhat.com> (raw)
In-Reply-To: <1565782781938.37795@mentor.com>

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

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 11:39 Schmid, Carsten
2019-08-14 12:54 ` Hans de Goede [this message]
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

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=15aa45c7-6e45-d03f-9336-4291f8b2dc66@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=Carsten_Schmid@mentor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    /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

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