* Re: [PATCH v1] PCI: keystone: Fix OF node reference leak in init
2026-06-30 20:29 [PATCH v1] PCI: keystone: Fix OF node reference leak in init Yuho Choi
@ 2026-06-30 20:41 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-30 20:41 UTC (permalink / raw)
To: Yuho Choi; +Cc: linux-pci
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
^ permalink raw reply [flat|nested] 2+ messages in thread