[v5] usb: hcd: use managed device resources

Commit Message

Schmid, Carsten Aug. 23, 2019, 2:11 p.m. UTC

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 <[e-mail]>
Tested-by: Carsten Schmid <[e-mail]>
---
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(-)

Comments

Greg Kroah-Hartman Aug. 25, 2019, 8:29 a.m. UTC | #1

On Fri, Aug 23, 2019 at 02:11:28PM +0000, Schmid, Carsten wrote:
> 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 <[e-mail]>
> Tested-by: Carsten Schmid <[e-mail]>
> ---
> 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.

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

Schmid, Carsten Aug. 26, 2019, 7:20 a.m. UTC | #2

>> 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

Mathias Nyman Aug. 26, 2019, 7:28 a.m. UTC | #3

On 25.8.2019 11.29, Greg KH wrote:
> On Fri, Aug 23, 2019 at 02:11:28PM +0000, Schmid, Carsten wrote:
>> 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 <[e-mail]>
>> Tested-by: Carsten Schmid <[e-mail]>
>> ---
>> 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.
> 
> 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

Schmid, Carsten Aug. 26, 2019, 8:09 a.m. UTC | #4

>>> 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
>
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 Aug. 26, 2019, 8:41 a.m. UTC | #5

On 26.8.2019 11.09, Schmid, Carsten wrote:
>>>> 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
>>
> 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 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 <[e-mail]>
Fixes: fa31b3cb2ae1 ("xhci: Add Intel extended cap / otg phy mux handling")
Cc: <[e-mail]> # v4.19+

Schmid, Carsten Aug. 26, 2019, 9:18 a.m. UTC | #6

> 
> 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 <[e-mail]>
> Fixes: fa31b3cb2ae1 ("xhci: Add Intel extended cap / otg phy mux handling")
> Cc: <[e-mail]> # v4.19+

Anything else needed from my side?
Please let me know if so.

Best regards
Carsten

Patch

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);
 }