usb: xhci-pci: reorder removal to avoid use-after-free
Subsystems (2)
PATCH RESEND v3 1/1 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 resource 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.
---
v2:
- more speaking name for private data element
- consider failure in driver init sequence
- fix minor issues found by checkpatch.pl
v3:
- corrected typo: resorce -> resource
- added Reviewed by and Tested-by
Resend v3:
- made a mistake when copying patch content to mail in v3
--- 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
drivers/usb/host/xhci-ext-caps.c +15 -10
@@ -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);
drivers/usb/host/xhci-pci.c +7 -1
@@ -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);
drivers/usb/host/xhci-pci.h new file +20 -0
@@ -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 */
drivers/usb/host/xhci.h +1 -0
@@ -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);
Signature · 1 line
2.17.1
Quoted 9 lines · Carsten
On driver removal, the
platform_device_unregistercall attached throughdevm_add_action_or_resetwas executed afterusb_hcd_pci_remove.
This lead to a use-after-free for the iomem resource 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.
Could all this be avoided if usb_hcd_pci_probe() used managed device resources as well?
(using devm_request_mem_region(), and devm_ioremap_nocache())
This way the iomem resource would be added to the same devres list as the platform_unregister_call, and the iomem resource should be released after the platform_device_unregister as devres_release_all() releases the resources in reverse order.
-Mathias
Hi,
Quoted 11 lines · Mathias Nyman
Quoted 9 lines · Carsten
On driver removal, the
platform_device_unregistercall attached throughdevm_add_action_or_resetwas executed afterusb_hcd_pci_remove.
This lead to a use-after-free for the iomem resource 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.
Quoted 8 lines
Could all this be avoided if
usb_hcd_pci_probe()used managed device resources as well?
(usingdevm_request_mem_region(), anddevm_ioremap_nocache())This way the iomem resource would be added to the same devres list as the
platform_unregister_call, and the iomem resource should be released after theplatform_device_unregisterasdevres_release_all()releases the resources in reverse order.
Yes I believe that that would work.
Regards,
Hans
Quoted 16 lines · Mathias Nyman
Quoted 15 lines · Carsten
Quoted 9 lines
On driver removal, the
platform_device_unregistercall attached throughdevm_add_action_or_resetwas executed afterusb_hcd_pci_remove.
This lead to a use-after-free for the iomem resource 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.
Could all this be avoided if
usb_hcd_pci_probe()used managed device resources as well?
(usingdevm_request_mem_region(), anddevm_ioremap_nocache())
Quoted 7 lines
Quoted 4 lines
This way the iomem resource would be added to the same devres list as the
platform_unregister_call, and the iomem resource should be released after theplatform_device_unregisterasdevres_release_all()releases the resources in reverse order.Yes I believe that that would work.
I don't think so, because xhci_create_intel_xhci_sw_pdev registers it's resource through platform_device_add_resources which does not use devm_.
The only thing what is done through devm in xhci_create_intel_xhci_sw_pdev is ret = devm_add_action_or_reset(...)
Carsten
Hi,
Quoted 21 lines · Carsten
Quoted 21 lines · Mathias Nyman
Quoted 19 lines · Carsten
Quoted 9 lines
On driver removal, the
platform_device_unregistercall attached throughdevm_add_action_or_resetwas executed afterusb_hcd_pci_remove.
This lead to a use-after-free for the iomem resource 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.
Could all this be avoided if
usb_hcd_pci_probe()used managed device resources as well?
(usingdevm_request_mem_region(), anddevm_ioremap_nocache())This way the iomem resource would be added to the same devres list as the
platform_unregister_call, and the iomem resource should be released after theplatform_device_unregisterasdevres_release_all()releases the resources in reverse order.
Quoted 7 lines
Quoted 2 lines
Yes I believe that that would work.
I don't think so, because
xhci_create_intel_xhci_sw_pdevregisters it's resource throughplatform_device_add_resourceswhich does not usedevm_.The only thing what is done through devm in
xhci_create_intel_xhci_sw_pdevis ret =devm_add_action_or_reset(...)
Right, so if I understand correctly the problem with the current code (before your patch) is:
Probe:
1. usb_hcd_pci_probe() uses request_mem_region()
2. xhci_create_intel_xhci_sw_pde uses platform_device_add_resources() which are a child of the previous region
Remove:
3. usb_hcd_pci_remove does release_region() while the child region is still in use
4. At end of remove() devm code calls xhci_intel_unregister_pdev
And the problem is we want to swap 3 and 4.
Now if we make usb_hcd_pci_probe() use devm_request_mem_region and drop the cleanup from usb_hcd_pci_remove
Probe:
1. usb_hcd_pci_probe() uses devm_request_mem_region(), this registers a release_region() with devm as cleanup
2. xhci_create_intel_xhci_sw_pde uses platform_device_add_resources() which are a child of the previous region and calls devm_add_action_or_reset() to add xhci_intel_unregister_pdev as cleanup
Remove:
3. usb_hcd_pci_remove does nothing of relevance
4. At end of remove() devm code runs and calls the cleanups in reverse order of registration! so it calls:
xhci_intel_unregister_pdev()
release_region()
Note the trick here is that the devm framework calls devm registered cleanup functions in reverse order of their registration, putting things in the right order.
Regards,
Hans