lkml.org 
[lkml]   [2019]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2] usb: xhci-pci: reorder removal to avoid use-after-free
    From
    Date
    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);
    >

    \
     
     \ /
      Last update: 2019-08-15 17:28    [W:4.365 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site