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 using devm for hcd-pci created resources.
This simplifies error handling on driver initialisation
and fixes the removal sequence.

Signed-off-by: Carsten Schmid <carsten_sch...@mentor.com>
Tested-by: Carsten Schmid <carsten_sch...@mentor.com>
---
Rationale:
Use-after-free was reproduced on 4.14.102 and 4.14.129 kernel
using unbind mechanism.
echo 0000:00:15.0 > /sys/bus/pci/drivers/xhci_hcd/unbind

Upstream version of driver is identical in the affected code.
Fix was tested successfully on 4.14.129.
Provided patch applies and compiles on v5.2.8 stable.
As this is a bugfix, please consider it to go to stable trees too.
---
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
v4:
   - completely rewritten to use devm resource allocation
     in hcd-pci
   - modified title to better reflect change content
   - removed Reviewed-by
     [old title: usb: xhci-pci: reorder removal to avoid use-after-free]
---
 drivers/usb/core/hcd-pci.c | 30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 03432467b05f..7537681355f6 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -216,17 +216,18 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
                /* EHCI, OHCI */
                hcd->rsrc_start = pci_resource_start(dev, 0);
                hcd->rsrc_len = pci_resource_len(dev, 0);
-               if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
-                               driver->description)) {
+               if (!devm_request_mem_region(&dev->dev, hcd->rsrc_start,
+                               hcd->rsrc_len, driver->description)) {
                        dev_dbg(&dev->dev, "controller already in use\n");
                        retval = -EBUSY;
                        goto put_hcd;
                }
-               hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
+               hcd->regs = devm_ioremap_nocache(&dev->dev, hcd->rsrc_start,
+                               hcd->rsrc_len);
                if (hcd->regs == NULL) {
                        dev_dbg(&dev->dev, "error mapping memory\n");
                        retval = -EFAULT;
-                       goto release_mem_region;
+                       goto put_hcd;
                }
 
        } else {
@@ -240,8 +241,8 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
 
                        hcd->rsrc_start = pci_resource_start(dev, region);
                        hcd->rsrc_len = pci_resource_len(dev, region);
-                       if (request_region(hcd->rsrc_start, hcd->rsrc_len,
-                                       driver->description))
+                       if (devm_request_region(&dev->dev, hcd->rsrc_start,
+                                       hcd->rsrc_len, driver->description))
                                break;
                }
                if (region == PCI_ROM_RESOURCE) {
@@ -275,20 +276,13 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct 
pci_device_id *id)
        }
 
        if (retval != 0)
-               goto unmap_registers;
+               goto put_hcd;
        device_wakeup_enable(hcd->self.controller);
 
        if (pci_dev_run_wake(dev))
                pm_runtime_put_noidle(&dev->dev);
        return retval;
 
-unmap_registers:
-       if (driver->flags & HCD_MEMORY) {
-               iounmap(hcd->regs);
-release_mem_region:
-               release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
-       } else
-               release_region(hcd->rsrc_start, hcd->rsrc_len);
 put_hcd:
        usb_put_hcd(hcd);
 disable_pci:
@@ -347,14 +341,6 @@ void usb_hcd_pci_remove(struct pci_dev *dev)
                dev_set_drvdata(&dev->dev, NULL);
                up_read(&companions_rwsem);
        }
-
-       if (hcd->driver->flags & HCD_MEMORY) {
-               iounmap(hcd->regs);
-               release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
-       } else {
-               release_region(hcd->rsrc_start, hcd->rsrc_len);
-       }
-
        usb_put_hcd(hcd);
        pci_disable_device(dev);
 }
-- 
2.17.1

Reply via email to