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

[Hans Zhang]

On 2026/3/11 06:37, Bjorn Helgaas wrote:
> 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.

Hi Bjorn,

Thank you very much for your reply. The V8 series will be sent to you 
for review shortly. If there are any issues, I will make the corrections 
again.

Best regards,
Hans