usb: hcd: use managed device resources
PATCH v5 1/1 usb: hcd: use managed device resources
Using managed device resources in usb_hcd_pci_probe() allows devm usage for resource subranges, such as the mmio resource for the platform device created to control host/device mode mux, which is a xhci extended capability, and sits inside the xhci mmio region.
If managed device resources are not used then "parent" resource is released before subrange at driver removal as .remove callback is called before the devres list of resources for this device is walked and released.
This has been observed with the xhci extended capability driver causing a use-after-free which is now fixed.
An additional nice benefit is that error handling on driver initialisation is simplified much.
---
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 also 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]
v5:
- rewrite commit message to cover all aspects of the patch [old title: usb: hcd: fix use-after-free on driver removal]
Thanks to Hans and Mathias for their encouraging support.
--- drivers/usb/core/hcd-pci.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-)
drivers/usb/core/hcd-pci.c +8 -22
@@ -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); }
Signature · 1 line
2.17.1
Greg KH
maintainerQuoted 24 lines · Carsten
Using managed device resources in
usb_hcd_pci_probe()allows devm usage for resource subranges, such as the mmio resource for the platform device created to control host/device mode mux, which is a xhci extended capability, and sits inside the xhci mmio region.If managed device resources are not used then "parent" resource is released before subrange at driver removal as .remove callback is called before the devres list of resources for this device is walked and released.
This has been observed with the xhci extended capability driver causing a use-after-free which is now fixed.
An additional nice benefit is that error handling on driver initialisation is simplified much.
Signed-off-by Carsten Schmid <[email protected]>Tested-by Carsten Schmid <[email protected]>---
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
Quoted 4 lines
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 also a bugfix, please consider it to go to stable trees too.
How far back should it go, just 4.14? Was this caused by a specific commit that you happened to notice?
thanks,
greg k-h
Quoted 11 lines
Quoted 4 lines
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 also a bugfix, please consider it to go to stable trees too.How far back should it go, just 4.14? Was this caused by a specific commit that you happened to notice?
thanks,
greg k-h
Looks like the ext caps driver has been introduced in 4.17-rc1 and backported to 4.14.97. (at least my git history tells so).
4.9 doesn't have it.
So, yes, 4.14 is the "oldest" candidate.
Thanks and best regards
Carsten
Quoted 25 lines · Greg KH
Quoted 24 lines · Carsten
Using managed device resources in
usb_hcd_pci_probe()allows devm usage for resource subranges, such as the mmio resource for the platform device created to control host/device mode mux, which is a xhci extended capability, and sits inside the xhci mmio region.If managed device resources are not used then "parent" resource is released before subrange at driver removal as .remove callback is called before the devres list of resources for this device is walked and released.
This has been observed with the xhci extended capability driver causing a use-after-free which is now fixed.
An additional nice benefit is that error handling on driver initialisation is simplified much.
Signed-off-by Carsten Schmid <[email protected]>Tested-by Carsten Schmid <[email protected]>---
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
Quoted 8 lines
Quoted 4 lines
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 also a bugfix, please consider it to go to stable trees too.How far back should it go, just 4.14? Was this caused by a specific commit that you happened to notice?
To me it looks like the causing commit was added to 4.17:
fa31b3c xhci: Add Intel extended cap / otg phy mux handling
Carsten, was the issue reproduced on upstream linux stable 4.14.129, or on some custom tree with backports?
-Mathias
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
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 also a bugfix, please consider it to go to stable trees too.How far back should it go, just 4.14? Was this caused by a specific commit that you happened to notice?
Quoted 8 lines
To me it looks like the causing commit was added to 4.17:
fa31b3c xhci: Add Intel extended cap / otg phy mux handlingCarsten, was the issue reproduced on upstream linux stable 4.14.129, or on some custom tree with backports?
-Mathias
The issue was reproduced on a custom kernel.
The commit you give "xhci: Add Intel extended cap / otg phy mux handling" is part of a merge from 4.14/usb to 4.14/yocto in this custom kernel. Hard to tell exactly where it came in.
Anyway, you are right, looks like upstream 4.14 doesn't have it. So 4.19 seems to be the oldest one, indeed.
For me, i am fine to know that the patch will go upstream and avoid curious crashes through this use-after-free in the future. I'm pretty sure that this could be reproduced with the upstream kernel as the mechanism is deterministic.
In our project, due to a crash that happened short before SOP and i did not have time to figure out where the problem originally came from, i did a really dirty hack setting the num_resource=0 of the platform device prior to platform removal code. Helps but is really, really dirty. And now as you accept the patch for upstream i can replace the dirty hack by the official patch for next SOP.
Thanks for all your guidance and efforts.
Carsten
Mathias Nyman
Reviewed-byQuoted 29 lines · Carsten
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
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 also a bugfix, please consider it to go to stable trees too.How far back should it go, just 4.14? Was this caused by a specific commit that you happened to notice?
Quoted 8 lines
To me it looks like the causing commit was added to 4.17:
fa31b3c xhci: Add Intel extended cap / otg phy mux handlingCarsten, was the issue reproduced on upstream linux stable 4.14.129, or on some custom tree with backports?
-Mathias
The issue was reproduced on a custom kernel.
The commit you give "xhci: Add Intel extended cap / otg phy mux handling" is part of a merge from 4.14/usb to 4.14/yocto in this custom kernel. Hard to tell exactly where it came in.Anyway, you are right, looks like upstream 4.14 doesn't have it. So 4.19 seems to be the oldest one, indeed.
For me, i am fine to know that the patch will go upstream and avoid curious crashes through this use-after-free in the future. I'm pretty sure that this could be reproduced with the upstream kernel as the mechanism is deterministic.
Quoted 7 lines
In our project, due to a crash that happened short before SOP and i did not have time to figure out where the problem originally came from, i did a really dirty hack setting the num_resource=0 of the platform device prior to platform removal code. Helps but is really, really dirty. And now as you accept the patch for upstream i can replace the dirty hack by the official patch for next SOP.
Thanks for fixing this, it solves a real upstream xhci related issue since 4.19. Fix is outside xhci so accepting this is no longer up to me, but for Greg:
Quoted 7 lines
Thanks for fixing this, it solves a real upstream xhci related issue since 4.19. Fix is outside xhci so accepting this is no longer up to me, but for Greg:
Reviewed-by Mathias Nyman <[email protected]>Fixes fa31b3cb2ae1 ("xhci: Add Intel extended cap / otg phy mux handling")Cc <[email protected]> # v4.19+
Anything else needed from my side?
Please let me know if so.
Best regards
Carsten