Re: [PATCH 2/2] PCI: Allow reusing PCI domain numbers in DT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] PCI: NULL pointer dereference in of_pci_bus_release_domain_nr()
- [Medium] PCI: dynamic domain ID leak in of_pci_bus_release_domain_nr()
- [Medium] PCI: uniqueness check bypassed in of_pci_bus_find_domain_nr()
--

commit 6f168ee04fb909d8010153d5588cf34c9e1e6db6
Author: Cui Chao <cuichao1753@phytium.com.cn>

PCI: Allow reusing PCI domain numbers in DT

This commit adds support for the linux,pci-domain-reuse Device Tree property
to allow multiple PCIe host bridges with disjoint physical resources to share
the same static linux,pci-domain number.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f80..9cb3132673f86 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6738,13 +6738,19 @@ static int of_pci_bus_find_domain_nr(struct device *parent)
>  		/*
>  		 * If domain is in DT, allocate it in static IDA.  This
>  		 * prevents duplicate static allocations in case of errors
> -		 * in DT.
> +		 * in DT. Platform that legitimately reuse a domain number
> +		 * across host bridges (disjoint ECAM/bus/memory/IO) opt out
> +		 * of this check with "linux,pci-domain-reuse".
>  		 */
>  		domain_nr = of_get_pci_domain_nr(parent->of_node);
> -		if (domain_nr >= 0)
> +		if (domain_nr >= 0) {
> +			if (of_property_read_bool(parent->of_node,
> +							"linux,pci-domain-reuse"))
> +				return domain_nr;
>  			return ida_alloc_range(&pci_domain_nr_static_ida,
>  					       domain_nr, domain_nr,
>  					       GFP_KERNEL);
> +		}
>  	}

[Severity: Medium]
The commit message states that for a PCI node that does not declare
linux,pci-domain-reuse, the original uniqueness check remains in place, and
that each RC node participating in reuse declares linux,pci-domain-reuse.

In drivers/pci/pci.c:of_pci_bus_find_domain_nr(), returning domain_nr
directly without recording or checking anything in pci_domain_nr_static_ida
means that if one host bridge declares linux,pci-domain = <0> with
linux,pci-domain-reuse and a second host bridge declares
linux,pci-domain = <0> without linux,pci-domain-reuse, both host bridges
succeed regardless of probe order.

If the reusing bridge probes first, domain 0 is never allocated in
pci_domain_nr_static_ida, so the non-reusing bridge's call to
ida_alloc_range(&pci_domain_nr_static_ida, 0, 0, GFP_KERNEL) succeeds. If the
non-reusing bridge probes first, it allocates domain 0 in
pci_domain_nr_static_ida, and the reusing bridge then bypasses
pci_domain_nr_static_ida completely.

Doesn't this allow a host bridge without linux,pci-domain-reuse to silently
share its domain number with another bridge?

>  
>  	/*
> @@ -6761,6 +6767,10 @@ static void of_pci_bus_release_domain_nr(struct device *parent, int domain_nr)
>  	if (domain_nr < 0)
>  		return;
>  
> +	/* Reusers never acquire the static slot, so never release it */
> +	if (of_property_read_bool(parent->of_node, "linux,pci-domain-reuse"))
> +		return;
> +
>  	/* Release domain from IDA where it was allocated. */
>  	if (parent && of_get_pci_domain_nr(parent->of_node) == domain_nr)
>  		ida_free(&pci_domain_nr_static_ida, domain_nr);

[Severity: High]
Can this dereference a NULL parent pointer in
drivers/pci/pci.c:of_pci_bus_release_domain_nr()?

When a root bus is created without a parent device (for example via
pci_create_root_bus(NULL, ...) or pci_scan_bus()), of_pci_bus_find_domain_nr()
checks if (parent) and allocates a dynamic domain number from
pci_domain_nr_dynamic_ida.

If pci_register_host_bridge() fails and unwinds, or when
pci_remove_root_bus() tears down the bus:

pci_register_host_bridge() / pci_remove_root_bus()
  pci_bus_release_domain_nr(NULL, bus->domain_nr)
    of_pci_bus_release_domain_nr(NULL, domain_nr)

Evaluating parent->of_node in of_property_read_bool(parent->of_node, ...)
happens before the existing if (parent && ...) check below (which was added in
commit f7245901de89 ("PCI: Avoid NULL dereference in
of_pci_bus_release_domain_nr()")), causing a NULL pointer dereference.

[Severity: Medium]
Also in drivers/pci/pci.c:of_pci_bus_release_domain_nr(), does returning early
on linux,pci-domain-reuse before checking of_get_pci_domain_nr(parent->of_node)
== domain_nr leak a dynamic domain ID from pci_domain_nr_dynamic_ida?

In drivers/pci/pci.c:of_pci_bus_find_domain_nr():

	if (parent) {
		domain_nr = of_get_pci_domain_nr(parent->of_node);
		if (domain_nr >= 0) {
			if (of_property_read_bool(parent->of_node,
							"linux,pci-domain-reuse"))
				return domain_nr;
			...
		}
	}
	return ida_alloc(&pci_domain_nr_dynamic_ida, GFP_KERNEL);

If a DT node specifies linux,pci-domain-reuse without a valid
linux,pci-domain property (so domain_nr < 0), of_pci_bus_find_domain_nr()
falls through and allocates domain_nr from pci_domain_nr_dynamic_ida.

When of_pci_bus_release_domain_nr() is later called on error or removal,
checking of_property_read_bool(parent->of_node, "linux,pci-domain-reuse")
before verifying that domain_nr came from of_get_pci_domain_nr(parent->of_node)
returns early instead of freeing domain_nr from pci_domain_nr_dynamic_ida in
the else branch:

	if (parent && of_get_pci_domain_nr(parent->of_node) == domain_nr) {
		if (!of_property_read_bool(parent->of_node,
					   "linux,pci-domain-reuse"))
			ida_free(&pci_domain_nr_static_ida, domain_nr);
	} else {
		ida_free(&pci_domain_nr_dynamic_ida, domain_nr);
	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102095150.2370959-1-cuichao1753@phytium.com.cn?part=2