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

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

Pre-existing issues:
- [High] Optional AER error handler callbacks are invoked unconditionally without being checked for existence, leading to a NULL pointer dereference.
- [High] Unsafe lockless access to `pcidev->dev.driver` leads to a TOCTOU race condition and Use-After-Free against driver unbinding.
--

--- Patch [1]: [PATCH v2] xen/pcifront: Fix PCI device reference leak in AER handling ---
commit e7640edce237b4072d2fc35f47ae61c01c1ebc16
Author: Ruoyu Wang <[email protected]>
Subject: xen/pcifront: Fix PCI device reference leak in AER handling

This commit fixes a PCI device reference leak in the Xen PCI frontend
driver's AER handling. It declares the looked-up device with the
__free(pci_dev_put) cleanup attribute. This ensures the reference is properly
dropped on all return paths.

Link: https://lore.kernel.org/r/[email protected]/

> diff --git a/drivers/pci/xen-pcifront.c b/drivers/pci/xen-pcifront.c
> index cffc32d660327..0dea8a69fa36c 100644
> --- a/drivers/pci/xen-pcifront.c
> +++ b/drivers/pci/xen-pcifront.c
> @@ -579,16 +579,15 @@ static pci_ers_result_t pcifront_common_process(int cmd,
>  	int bus = pdev->sh_info->aer_op.bus;
>  	int devfn = pdev->sh_info->aer_op.devfn;
>  	int domain = pdev->sh_info->aer_op.domain;
> -	struct pci_dev *pcidev;
> +	struct pci_dev *pcidev __free(pci_dev_put) =
> +		pci_get_domain_bus_and_slot(domain, bus, devfn);
>  
>  	dev_dbg(&pdev->xdev->dev,
>  		"pcifront AER process: cmd %x (bus:%x, devfn%x)",
>  		cmd, bus, devfn);
>  
> -	pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
>  	if (!pcidev || !pcidev->dev.driver) {
>  		dev_err(&pdev->xdev->dev, "device or AER driver is NULL\n");
> -		pci_dev_put(pcidev);
>  		return PCI_ERS_RESULT_NONE;
>  	}
>  	pdrv = to_pci_driver(pcidev->dev.driver);

[Severity: High]
This isn't a bug introduced by this patch, but does accessing
pcidev->dev.driver locklessly here introduce a race condition?

If a device unbind races with an incoming AER event, to_pci_driver(__drv)
might evaluate pcidev->dev.driver to NULL on its second access, or the
driver could be unloaded before callbacks are invoked. Standard PCI AER
handlers usually hold device_lock(&dev->dev) to serialize against driver
binding and unbinding.

[Severity: High]
This is a pre-existing issue, but further down in pcifront_common_process(),
does the code invoke optional AER callbacks without checking if they exist?

While the error_detected callback is verified, optional callbacks appear to
be invoked unconditionally:

    case XEN_PCI_OP_aer_mmio:
        return pdrv->err_handler->mmio_enabled(pcidev);
    case XEN_PCI_OP_aer_slotreset:
        return pdrv->err_handler->slot_reset(pcidev);

If a bound driver lacks these optional callbacks, will processing these
commands from the backend result in a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1