Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [Patch v5] usb: hcd: use managed device resources
@ 2019-08-23 14:11 Schmid, Carsten
  2019-08-25  8:29 ` Greg KH
  0 siblings, 1 reply; 7+ messages in thread
From: Schmid, Carsten @ 2019-08-23 14:11 UTC (permalink / raw)
  To: Mathias Nyman, Hans de Goede; +Cc: linux-usb

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 <carsten_schmid@mentor.com>
Tested-by: Carsten Schmid <carsten_schmid@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 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(-)

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch v5] usb: hcd: use managed device resources
  2019-08-23 14:11 [Patch v5] usb: hcd: use managed device resources Schmid, Carsten
@ 2019-08-25  8:29 ` Greg KH
  2019-08-26  7:20   ` AW: " Schmid, Carsten
  2019-08-26  7:28   ` Mathias Nyman
  0 siblings, 2 replies; 7+ messages in thread
From: Greg KH @ 2019-08-25  8:29 UTC (permalink / raw)
  To: Schmid, Carsten; +Cc: Mathias Nyman, Hans de Goede, linux-usb

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 <carsten_schmid@mentor.com>
> Tested-by: Carsten Schmid <carsten_schmid@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 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* AW: [Patch v5] usb: hcd: use managed device resources
  2019-08-25  8:29 ` Greg KH
@ 2019-08-26  7:20   ` " Schmid, Carsten
  2019-08-26  7:28   ` Mathias Nyman
  1 sibling, 0 replies; 7+ messages in thread
From: Schmid, Carsten @ 2019-08-26  7:20 UTC (permalink / raw)
  To: Greg KH; +Cc: Mathias Nyman, Hans de Goede, linux-usb

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Patch v5] usb: hcd: use managed device resources
  2019-08-25  8:29 ` Greg KH
  2019-08-26  7:20   ` AW: " Schmid, Carsten
@ 2019-08-26  7:28   ` Mathias Nyman
  2019-08-26  8:09     ` AW: " Schmid, Carsten
  1 sibling, 1 reply; 7+ messages in thread
From: Mathias Nyman @ 2019-08-26  7:28 UTC (permalink / raw)
  To: Greg KH, Schmid, Carsten; +Cc: Hans de Goede, linux-usb

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 <carsten_schmid@mentor.com>
>> Tested-by: Carsten Schmid <carsten_schmid@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 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* AW: [Patch v5] usb: hcd: use managed device resources
  2019-08-26  7:28   ` Mathias Nyman
@ 2019-08-26  8:09     ` " Schmid, Carsten
  2019-08-26  8:41       ` Mathias Nyman
  0 siblings, 1 reply; 7+ messages in thread
From: Schmid, Carsten @ 2019-08-26  8:09 UTC (permalink / raw)
  To: Mathias Nyman, Greg KH; +Cc: Hans de Goede, linux-usb

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: AW: [Patch v5] usb: hcd: use managed device resources
  2019-08-26  8:09     ` AW: " Schmid, Carsten
@ 2019-08-26  8:41       ` Mathias Nyman
  2019-08-26  9:18         ` AW: " Schmid, Carsten
  0 siblings, 1 reply; 7+ messages in thread
From: Mathias Nyman @ 2019-08-26  8:41 UTC (permalink / raw)
  To: Schmid, Carsten, Greg KH; +Cc: Hans de Goede, linux-usb

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 <mathias.nyman@linux.intel.com>
Fixes: fa31b3cb2ae1 ("xhci: Add Intel extended cap / otg phy mux handling")
Cc: <stable@vger.kernel.org> # v4.19+


^ permalink raw reply	[flat|nested] 7+ messages in thread

* AW: AW: [Patch v5] usb: hcd: use managed device resources
  2019-08-26  8:41       ` Mathias Nyman
@ 2019-08-26  9:18         ` " Schmid, Carsten
  0 siblings, 0 replies; 7+ messages in thread
From: Schmid, Carsten @ 2019-08-26  9:18 UTC (permalink / raw)
  To: Mathias Nyman, Greg KH; +Cc: Hans de Goede, linux-usb

> 
> 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 <mathias.nyman@linux.intel.com>
> Fixes: fa31b3cb2ae1 ("xhci: Add Intel extended cap / otg phy mux handling")
> Cc: <stable@vger.kernel.org> # v4.19+

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

Best regards
Carsten

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 14:11 [Patch v5] usb: hcd: use managed device resources Schmid, Carsten
2019-08-25  8:29 ` Greg KH
2019-08-26  7:20   ` AW: " Schmid, Carsten
2019-08-26  7:28   ` Mathias Nyman
2019-08-26  8:09     ` AW: " Schmid, Carsten
2019-08-26  8:41       ` Mathias Nyman
2019-08-26  9:18         ` AW: " Schmid, Carsten

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox