Re: [PATCH] s390/pci: Fix missing pci_dev_put() in zpci_report_status()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless access to `pdev->dev.driver` and double-fetch (TOCTOU) leading to a kernel panic or use-after-free
--

--- Patch [1]: [PATCH] s390/pci: Fix missing pci_dev_put() in zpci_report_status() ---
commit 22a605e88c1bebfc9551790e5d42753aee881365
Author: Niklas Schnelle <schnelle@linux.ibm.com>

s390/pci: Fix missing pci_dev_put() in zpci_report_status()

In zpci_report_status() a reference to the struct pci_dev associated
with the struct zpci_dev being reported about is acquired using
pci_get_slot(). This reference needs to be dropped with pci_dev_put()
but this call is missing thus leaking the reference. On subsequent hot
unplug this will cause the struct pci_dev to not be released leaking
memory and potentially prevent reattach.

> diff --git a/arch/s390/pci/pci_report.c b/arch/s390/pci/pci_report.c
> index 7030f7052926a..76b8afc146fea 100644
> --- a/arch/s390/pci/pci_report.c
> +++ b/arch/s390/pci/pci_report.c
> @@ -151,6 +151,7 @@ int zpci_report_status(struct zpci_dev *zdev, const char *operation, const char
>  	else
>  		pr_info("Reported PCI device status\n");
>  
> +	pci_dev_put(pdev);

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential race
condition earlier in this function when accessing the driver name?

In zpci_report_status(), pdev->dev.driver is read locklessly:

	if (pdev)
		driver = to_pci_driver(pdev->dev.driver);

The to_pci_driver() macro evaluates its argument twice. If a concurrent
driver unbind clears pdev->dev.driver between these evaluations, it could
pass NULL to container_of_const(), yielding a bogus negative pointer offset.

This pointer is then dereferenced later in the function:

	buf += scnprintf(buf, end - buf, "driver: %s\n",
			 (driver) ? driver->name : "n/a");

Because the negative pointer offset is non-zero, this check would falsely
evaluate to true and dereference the invalid pointer, which could cause a
page fault.

Even if the macro evaluates safely, pci_get_slot() does not pin the driver
module. Could this result in a use-after-free when reading driver->name
if the module is unloaded concurrently?

It appears the caller explicitly drops the device lock before calling this
function:

arch/s390/pci/pci_event.c:zpci_event_attempt_error_recovery()
	...
out_unlock:
	device_unlock(&pdev->dev);
	zpci_report_status(zdev, "recovery", status_str);

Would it be safer to protect this access to prevent concurrent unbinds?

>  	free_page((unsigned long)report);
>  
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-fix_zpci_report_status_pdev_leak-v1-1-576520f4d068@linux.ibm.com?part=1