| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3cb06e75-70a8-0f59-ae08-5bbd99abbead@redhat.com>
Date: Thu, 15 Aug 2019 17:27:24 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: "Schmid, Carsten" <Carsten_Schmid@...tor.com>
Cc: "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v2] usb: xhci-pci: reorder removal to avoid use-after-free
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@...tor.com>
Patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@...hat.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);
>
Powered by blists - more mailing lists