Re: [PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation

[Bjorn Helgaas]

On Sun, Mar 08, 2026 at 10:26:28PM +0800, Hans Zhang wrote:
> The of_pci_get_max_link_speed() function currently validates the
> "max-link-speed" DT property to be in the range 1..4 (Gen1..Gen4).
> This imposes a maintenance burden because each new PCIe generation
> would require updating this validation.
> 
> Remove the range check so the function returns the raw property value
> (or a negative error code if the property is missing or malformed).
> Callers must now validate the returned speed against the range they
> support.  A subsequent patch adds such validation to the DWC driver,
> which is the primary user of this function.
> 
> This change allows future PCIe generations to be supported without
> modifying drivers/pci/of.c.
> 
> Signed-off-by: Hans Zhang <18255117159@xxxxxxx>
> Acked-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>

Mani's right that we shouldn't have a window without any validation.
I hope I didn't otherwise.

> ---
>  drivers/pci/of.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 9f8eb5df279e..cff5fd337c2b 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -889,10 +889,11 @@ EXPORT_SYMBOL_GPL(of_pci_supply_present);
>  int of_pci_get_max_link_speed(struct device_node *node)
>  {
>  	u32 max_link_speed;
> +	int ret;
>  
> -	if (of_property_read_u32(node, "max-link-speed", &max_link_speed) ||
> -	    max_link_speed == 0 || max_link_speed > 4)
> -		return -EINVAL;
> +	ret = of_property_read_u32(node, "max-link-speed", &max_link_speed);
> +	if (ret)
> +		return ret;
>  
>  	return max_link_speed;
>  }

>From AI review (gemini/gemini-3.1-pro-preview):

  By removing the upper bounds check here, the returned max_link_speed
  value can now be arbitrarily large if configured incorrectly in the
  device tree. While the commit message notes that a subsequent patch
  adds validation to the DWC driver, there are several other callers
  in the tree that assume the returned value is bounded. These drivers
  remain vulnerable to out-of-bounds accesses and hardware
  misconfiguration at the end of this patch series.

  For example, in drivers/pci/controller/pcie-rzg3s-host.c, the
  returned value is used directly as an array index. Could this cause
  an out-of-bounds read on pcie_link_speed[]?

I think this is a valid concern, and I think we should protect
pcie_link_speed[] by making it static and accessing it via a function
that validates the index.  It's too hard to enforce validation at
every place that uses it.

I think you're going to have to:

  - Validate every use of dw_pcie.max_link_speed (set from
    of_pci_get_max_link_speed()) in the dwc glue drivers

  - Validate the return from of_pci_get_max_link_speed() at every
    other caller.  It looks like j721e_pcie_set_link_speed(),
    brcm_pcie_probe(), mtk_pcie_setup(), rzg3s_pcie_probe() would need
    that.

Right now we validate inside of_pci_get_max_link_speed(), which can
help avoid generic problems like indexing pcie_link_speed[], but it
can't account for individual driver restrictions.  So I do still think
you're right to move it from drivers/pci/of.c to the individual
drivers because those drivers are where it's really important, and it
avoids changing a common place for driver-specific reasons.  We're
just going to have to strictly enforce it in those drivers.