Re: [PATCH v1] PCI: keystone: Fix OF node reference leak in init

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbinding the `keystone-pcie` driver leaks the PCI root bus or Endpoint Controller (EPC) subsystem registration, leading to a Use-After-Free.
- [High] Unbinding the driver causes a kernel panic due to a NULL pointer dereference when tearing down device links for missing optional PHYs.
- [Medium] Missing `of_node_put(args.np)` after successful calls to `of_parse_phandle_with_fixed_args()` causes OF node reference leaks.
- [Medium] The `err_link` error path in `ks_pcie_probe()` leaks successfully allocated device links due to incorrect loop termination logic.
--

commit 7622e803c35b7f152e532145ef64528354f691a1
Author: Yuho Choi <dbgh9129@gmail.com>

PCI: keystone: Fix OF node reference leak in init

of_find_matching_node() returns a device node with its reference count
incremented. ks_pcie_init() only uses the returned node to decide whether
to register the ARM external abort fault handler, but never drops the
reference.

Store the lookup result in a temporary variable and release it with
of_node_put() once the existence check has been made.

> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 278d2dba1db08..f1b27aed488de 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -1389,13 +1389,17 @@ static int ks_pcie_fault(unsigned long addr, unsigned int fsr,
>  
>  static int __init ks_pcie_init(void)
>  {
> +	struct device_node *np;
>  	/*
>  	 * PCIe access errors that result into OCP errors are caught by ARM as
>  	 * "External aborts"
>  	 */
> -	if (of_find_matching_node(NULL, ks_pcie_of_match))
> +	np = of_find_matching_node(NULL, ks_pcie_of_match);
> +	if (np) {
> +		of_node_put(np);
>  		hook_fault_code(17, ks_pcie_fault, SIGBUS, 0,
>  				"Asynchronous external abort");
> +	}
>  
>  	return platform_driver_register(&ks_pcie_driver);
>  }

While reviewing this patch, I noticed a few pre-existing issues in the same
file.

> static int ks_pcie_init_id(struct keystone_pcie *ks_pcie)
> {
[ ... ]
> 	/* Do not error out to maintain old DT compatibility */
> 	ret = of_parse_phandle_with_fixed_args(np, "ti,syscon-pcie-id", 1, 0, &args);
> 	if (!ret)
> 		offset = args.args[0];

[Severity: Medium]
This is a pre-existing issue, but does this code leak the OF node reference
by failing to call of_node_put(args.np) after the property is parsed?

The same pattern appears to exist in ks_pcie_set_mode():

> static int ks_pcie_set_mode(struct device *dev)
> {
[ ... ]
> 	/* Do not error out to maintain old DT compatibility */
> 	ret = of_parse_phandle_with_fixed_args(np, "ti,syscon-pcie-mode", 1, 0, &args);
> 	if (!ret)
> 		offset = args.args[0];

And also in ks_pcie_am654_set_mode():

> static int ks_pcie_am654_set_mode(struct device *dev,
> 				  enum dw_pcie_device_mode mode)
> {
[ ... ]
> 	/* Do not error out to maintain old DT compatibility */
> 	ret = of_parse_phandle_with_fixed_args(np, "ti,syscon-pcie-mode", 1, 0, &args);
> 	if (!ret)
> 		offset = args.args[0];


> static int ks_pcie_probe(struct platform_device *pdev)
> {
[ ... ]
> err_link:
> 	while (--i >= 0 && link[i])
> 		device_link_del(link[i]);
> 
> 	return ret;
> }

[Severity: Medium]
This is a pre-existing issue, but does the && link[i] condition prematurely
terminate the cleanup loop? If probe fails after skipping an optional PHY,
leaving a NULL element at index 1 while index 0 has a valid link, would this
leak the device link at index 0?


> static void ks_pcie_remove(struct platform_device *pdev)
> {
> 	struct keystone_pcie *ks_pcie = platform_get_drvdata(pdev);
> 	struct device_link **link = ks_pcie->link;
> 	int num_lanes = ks_pcie->num_lanes;
> 	struct device *dev = &pdev->dev;
> 
> 	pm_runtime_put(dev);
> 	pm_runtime_disable(dev);
> 	ks_pcie_disable_phy(ks_pcie);

[Severity: High]
This is a pre-existing issue, but does this code leave the PCI root bus or
endpoint subsystem registered? Failing to call dw_pcie_host_deinit() or
dw_pcie_ep_deinit() before the driver structures are freed could result in a
use-after-free when unbinding the driver.

> 	while (num_lanes--)
> 		device_link_del(link[num_lanes]);
> }

[Severity: High]
This is a pre-existing issue, but can this while loop cause a NULL pointer
dereference? If an optional PHY was missing during ks_pcie_probe(),
link[num_lanes] could be NULL. Because device_link_del() does not check for
NULL before dereferencing, will unbinding the driver panic the kernel?

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