Re: [PATCH v11 10/10] PCI: of: Avoid np->data usage for the node changeset
From: sashiko-bot@kernel.org
Date: 2026-09-09 08:17:55
Also in:
linux-pci
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Missing NULL check on `np` in `of_pci_remove_node` and `of_pci_remove_host_bridge_node` leading to kernel panic. - [High] Dangling pointer in `pdev->cset` and `bridge->cset` after freeing, leading to potential use-after-free or double-free. -- commit 506858ef76ab83f59a005bd51f3fde0f4d73a6a2 Author: Herve Codina [off-list ref] PCI: of: Avoid np->data usage for the node changeset This patch modifies how OF changeset references are stored during PCI node creation. Instead of using np->data, which can be altered by platform code, it stores the changeset in pdev->cset or bridge->cset. This ensures the correct changeset is reverted and destroyed during node removal.
quoted hunk ↗ jump to hunk
diff --git a/drivers/pci/of.c b/drivers/pci/of.c index 0a5797652e183..cd87128c71ddc 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c@@ -732,15 +732,16 @@ void of_pci_remove_node(struct pci_dev *pdev) { struct device_node *np; - np = pci_device_to_OF_node(pdev); - if (!np || !of_node_check_flag(np, OF_DYNAMIC)) + if (!pdev->cset) return; + np = pci_device_to_OF_node(pdev); fw_devlink_set_device(&np->fwnode, NULL);
[Severity: High] Will this result in an invalid pointer dereference if np is NULL? The previous implementation had a NULL check for np, but the new code computes &np->fwnode unconditionally. If pdev->dev.of_node is cleared prior to removal, pci_device_to_OF_node() will return NULL, causing fw_devlink_set_device() to dereference an offset address.
device_remove_of_node(&pdev->dev); - of_changeset_revert(np->data); - of_changeset_destroy(np->data); + of_changeset_revert(pdev->cset); + of_changeset_destroy(pdev->cset); of_node_put(np); + kfree(pdev->cset);
[Severity: High] Is it possible for of_pci_remove_node() to be called twice for the same device? If so, leaving pdev->cset as a dangling pointer could lead to a double-free. Should pdev->cset be set to NULL after kfree() so that the earlier if (!pdev->cset) check can prevent a second execution? [ ... ]
quoted hunk ↗ jump to hunk
@@ -828,16 +828,17 @@ void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge) { struct device_node *np; - np = pci_bus_to_OF_node(bridge->bus); - if (!np || !of_node_check_flag(np, OF_DYNAMIC)) + if (!bridge->cset) return; + np = pci_bus_to_OF_node(bridge->bus); fw_devlink_set_device(&np->fwnode, NULL);
[Severity: High] Can np evaluate to NULL here? Similar to the issue in of_pci_remove_node(), if pci_bus_to_OF_node() returns NULL, the missing NULL check will cause a crash during the &np->fwnode pointer arithmetic.
device_remove_of_node(&bridge->bus->dev); device_remove_of_node(&bridge->dev); - of_changeset_revert(np->data); - of_changeset_destroy(np->data); + of_changeset_revert(bridge->cset); + of_changeset_destroy(bridge->cset); of_node_put(np); + kfree(bridge->cset);
[Severity: High] Could this lead to a use-after-free or double-free on bridge->cset? If a removal API like pci_stop_root_bus() is invoked multiple times, the dangling pointer bypasses the if (!bridge->cset) check at the start of the function, resulting in a double-free on the changeset. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260909080114.591938-1-herve.codina@bootlin.com?part=10