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

[Herve Codina <herve.codina@bootlin.com>]

On Wed, 09 Sep 2026 08:17:54 +0000
sashiko-bot@kernel.org wrote:

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

How np could be NULL with pdev->cset non-NULL ?

I don't think this can happen.

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

I don't think that of_pci_remove_node() can be called twice.

> 
> [ ... ]
> 
> > @@ -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.

bridge->cset protects agains a NULL np.

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

I don't think that pci_stop_root_bus() can be called twice.