[PATCH] xen/pcifront: Fix PCI device reference leak in AER handling

[PATCH] xen/pcifront: Fix PCI device reference leak in AER handling

[Ruoyu Wang]

pci_get_domain_bus_and_slot() increments the reference count of the
returned PCI device. pcifront_common_process() drops that reference only
when the device or its driver is missing. All paths for a bound device
either return directly after invoking an error recovery callback or fall
through without calling pci_dev_put(). Consequently, each AER request for
a bound device leaks a reference and can keep the device allocated after
removal.

Store the callback result, release the reference after callback dispatch,
and then return the result. This keeps the device alive while its callback
runs and balances the lookup on every successful path.

This issue was found by a static analysis checker and confirmed by manual
source review.

Fixes: 956a9202cd12 ("xen-pcifront: Xen PCI frontend driver.")
Signed-off-by: Ruoyu Wang <ruoyuw560@gmail.com>
---
 drivers/pci/xen-pcifront.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
index cffc32d6603277..07263dfe22d538 100644
--- a/drivers/pci/xen-pcifront.c
+++ b/drivers/pci/xen-pcifront.c
@@ -575,6 +575,7 @@ static pci_ers_result_t pcifront_common_process(int cmd,
 						struct pcifront_device *pdev,
 						pci_channel_state_t state)
 {
+	pci_ers_result_t result = PCI_ERS_RESULT_NONE;
 	struct pci_driver *pdrv;
 	int bus = pdev->sh_info->aer_op.bus;
 	int devfn = pdev->sh_info->aer_op.devfn;
@@ -597,21 +598,25 @@ static pci_ers_result_t pcifront_common_process(int cmd,
 		pci_dbg(pcidev, "trying to call AER service\n");
 		switch (cmd) {
 		case XEN_PCI_OP_aer_detected:
-			return pdrv->err_handler->error_detected(pcidev, state);
+			result = pdrv->err_handler->error_detected(pcidev, state);
+			break;
 		case XEN_PCI_OP_aer_mmio:
-			return pdrv->err_handler->mmio_enabled(pcidev);
+			result = pdrv->err_handler->mmio_enabled(pcidev);
+			break;
 		case XEN_PCI_OP_aer_slotreset:
-			return pdrv->err_handler->slot_reset(pcidev);
+			result = pdrv->err_handler->slot_reset(pcidev);
+			break;
 		case XEN_PCI_OP_aer_resume:
 			pdrv->err_handler->resume(pcidev);
-			return PCI_ERS_RESULT_NONE;
+			break;
 		default:
 			dev_err(&pdev->xdev->dev,
 				"bad request in aer recovery operation!\n");
 		}
 	}
 
-	return PCI_ERS_RESULT_NONE;
+	pci_dev_put(pcidev);
+	return result;
 }
 
 
-- 
2.51.0

Re: [PATCH] xen/pcifront: Fix PCI device reference leak in AER handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Accessing `pcidev->dev.driver` and invoking driver AER callbacks without holding `device_lock` risks a Use-After-Free due to concurrent driver unbinding.
- [High] Unconditional invocation of optional AER callbacks (`mmio_enabled`, `slot_reset`, `resume`) without verifying they are implemented by the driver.
--

--- Patch [1]: [PATCH] xen/pcifront: Fix PCI device reference leak in AER handling ---
commit 8d4be8a3c9e93de894d26cb69e9804d8f8ff8144
Author: Ruoyu Wang <ruoyuw560@gmail.com>

xen/pcifront: Fix PCI device reference leak in AER handling

This commit fixes a reference leak in pcifront_common_process by storing
the AER callback result, releasing the device reference, and then returning.
This keeps the device alive while its callback runs and balances the lookup
on every successful path.

> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c

[ ... ]

> @@ -597,21 +598,25 @@ static pci_ers_result_t pcifront_common_process(int cmd,
>  		pci_dbg(pcidev, "trying to call AER service\n");
>  		switch (cmd) {
>  		case XEN_PCI_OP_aer_detected:
> -			return pdrv->err_handler->error_detected(pcidev, state);
> +			result = pdrv->err_handler->error_detected(pcidev, state);
> +			break;

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to access the
driver pointer and invoke callbacks in this function without holding the
device lock?

The code earlier extracts the driver:

pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
if (!pcidev || !pcidev->dev.driver) {
	...
}
pdrv = to_pci_driver(pcidev->dev.driver);

If a concurrent unbind occurs (for example, via sysfs), can the driver be
unbound and freed between the check for pcidev->dev.driver and the callback
invocation? Core PCI error recovery typically holds the device lock to prevent
a use-after-free or NULL pointer dereference in this scenario.

>  		case XEN_PCI_OP_aer_mmio:
> -			return pdrv->err_handler->mmio_enabled(pcidev);
> +			result = pdrv->err_handler->mmio_enabled(pcidev);
> +			break;
>  		case XEN_PCI_OP_aer_slotreset:
> -			return pdrv->err_handler->slot_reset(pcidev);
> +			result = pdrv->err_handler->slot_reset(pcidev);
> +			break;

[Severity: High]
This is a pre-existing issue, but does this code assume all drivers
implement the optional AER callbacks?

The function checks if the error_detected callback is present before
entering the switch statement:

if (pdrv->err_handler && pdrv->err_handler->error_detected) {
	...
}

However, many PCI drivers do not implement mmio_enabled, slot_reset, or
resume. If the Xen backend forwards one of these events for a driver lacking
the specific callback, could invoking it here result in an immediate NULL
pointer dereference?

>  		case XEN_PCI_OP_aer_resume:
>  			pdrv->err_handler->resume(pcidev);
> -			return PCI_ERS_RESULT_NONE;
> +			break;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813153138.3953222-1-ruoyuw560@gmail.com?part=1