Re: [PATCH v10 10/10] PCI: of: Guard against node removal with incorrect np->data

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

Pre-existing issues:
- [High] Unsafe cast of `np->data` to `struct of_changeset *` without type validation, leading to type confusion and potential memory corruption.
--

--- Patch [10]: [PATCH v10 10/10] PCI: of: Guard against node removal with incorrect np->data ---
commit 3c2e9ae91c2c8b16b99779200dfab12179834af1
Author: Herve Codina <[email protected]>

PCI: of: Guard against node removal with incorrect np->data

This commit adds a check for !np->data in of_pci_remove_node() and
of_pci_remove_host_bridge_node() to prevent NULL pointer dereferences. It
protects against dynamic nodes created by device-tree overlays that do not
populate np->data.

> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 28896f748e8d3..9f35e1e07cdcd 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -654,7 +654,7 @@ 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 (!np || !of_node_check_flag(np, OF_DYNAMIC) || !np->data)
>  		return;
>  
>  	fw_devlink_set_device(&np->fwnode, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but does this still leave a type
confusion issue if np->data is populated by another subsystem?

While this patch prevents a NULL pointer dereference, it doesn't verify that a
non-NULL np->data actually points to a struct of_changeset. On some platforms
like PowerPC, the OF_RECONFIG_ATTACH_NODE notifier (e.g., in
pci_dn_reconfig_notifier()) intercepts node additions and populates np->data
with its own structure, such as a struct pci_dn.

If that happens, and the device is later removed, of_pci_remove_node() will
see a non-NULL np->data, proceed past this check, and pass the foreign pointer
to of_changeset_revert() and of_changeset_destroy() lower down in the function:

	fw_devlink_set_device(&np->fwnode, NULL);
	device_remove_of_node(&pdev->dev);
	of_changeset_revert(np->data);
	of_changeset_destroy(np->data);

Could this result in memory corruption or a kernel panic when the wrong type
is cast and dereferenced?

[ ... ]

> @@ -750,7 +750,7 @@ 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 (!np || !of_node_check_flag(np, OF_DYNAMIC) || !np->data)
>  		return;
>  
>  	fw_devlink_set_device(&np->fwnode, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but the same type confusion issue
seems to apply here in of_pci_remove_host_bridge_node(). 

If the host bridge node's np->data is populated by another subsystem with a
different structure, will passing it to of_changeset_revert() and
of_changeset_destroy() cause memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10