Hi,
On 22-08-19 17:48, Schmid, Carsten wrote:
On 22-08-19 17:23, Mathias Nyman wrote:
On 16.8.2019 12.03, Schmid, Carsten wrote:
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.
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.
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(...)
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
