Re: [PATCH v11 10/10] PCI: of: Avoid np->data usage for the node changeset

[sashiko-bot@kernel.org]

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 <herve.codina@bootlin.com>

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.

> 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?

[ ... ]

> @@ -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