Re: [PATCH] PCI: dwc: meson: Fix reset GPIO initial state

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

Pre-existing issues:
- [High] Missing .remove callback leads to Use-After-Free and resource leak on unbind/module unload.
--

commit 6f34b371980942ee76ca5383825dd8ecaff2cbfb
Author: Ronald Claveau <[email protected]>

PCI: dwc: meson: Fix reset GPIO initial state

This commit changes the initial state of the reset GPIO from GPIOD_OUT_LOW
to GPIOD_OUT_HIGH. This fixes the reset assertion logic that was inverted
by a previous commit.

> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 0694084f612b7..15ed59b8764fb 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -400,7 +400,7 @@ static int meson_pcie_probe(struct platform_device *pdev)
>  		return PTR_ERR(mp->phy);
>  	}
>  
> -	mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	mp->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);

[Severity: High]
This is a pre-existing issue, but does this driver lack a .remove callback
to clean up the PCIe host?

Looking at meson_pcie_probe(), it initializes the DWC PCIe host and
registers the PCI root bus:

drivers/pci/controller/dwc/pci-meson.c:meson_pcie_probe() {
    ...
    ret = dw_pcie_host_init(&pci->pp);
    if (ret < 0) {
    ...
}

However, meson_pcie_driver does not implement a .remove callback to call
dw_pcie_host_deinit(), nor does it set .suppress_bind_attrs = true.

When the driver is unbound via sysfs or unloaded, devres automatically frees
the memory for struct meson_pcie (and its embedded struct dw_pcie) and
disables clocks and resets.

Because dw_pcie_host_deinit() is never called, the PCIe root bus and its child
devices appear to remain registered in the PCI core.

Could this lead to a use-after-free and panic if subsequent interactions
with these leaked PCI devices occur (for example, via sysfs or lspci) after
the memory has been freed?

>  	if (IS_ERR(mp->reset_gpio)) {
>  		dev_err(dev, "get reset gpio failed\n");
>  		return PTR_ERR(mp->reset_gpio);

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic