1. Patchew
  2. linux
  3. [PATCH v7 0/2] PCI: of: Remove max-link-speed generation validation
  4. View patch

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

Hans Zhang posted 2 patches 1 month ago
There is a newer version of this series
[PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation
Posted by Hans Zhang 1 month ago 
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@163.com>
Acked-by: Manivannan Sadhasivam <mani@kernel.org>
---
 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;
 }
-- 
2.34.1
Re: [PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation
Posted by Bjorn Helgaas 4 weeks, 1 day ago 
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@163.com>
> Acked-by: Manivannan Sadhasivam <mani@kernel.org>

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.
Re: [PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation
Posted by Hans Zhang 3 weeks, 6 days ago 

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@163.com>
>> Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> 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
Re: [PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation
Posted by Manivannan Sadhasivam 1 month ago 
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@163.com>
> Acked-by: Manivannan Sadhasivam <mani@kernel.org>

I take my Ack back. If we decide to go with validating DTS properties (which is
fine though for non-resources such as clocks), then we should make sure that it
happens in a single patch.

Right now, you remove the check in one patch and add it back in another. That
leaves a window...

- Mani

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation
Posted by Hans Zhang 1 month ago 

On 2026/3/9 19:47, Manivannan Sadhasivam 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@163.com>
>> Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> 
> I take my Ack back. If we decide to go with validating DTS properties (which is
> fine though for non-resources such as clocks), then we should make sure that it
> happens in a single patch.
> 
> Right now, you remove the check in one patch and add it back in another. That
> leaves a window...
> 

Hi Mani,

For the second patch in this series, I made the changes based on Bjorn's 
comments. Maybe I misunderstood something.

Bjorn's reply:
https://lore.kernel.org/linux-pci/20260306170856.GA107733@bhelgaas/

May I ask if I should place the second patch on top of the first one? Is 
it just a matter of swapping the order of the two patches? Do you have 
any good methods?

Best regards,
Hans

> - Mani
>
Re: [PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation
Posted by Shawn Lin 1 month ago 
在 2026/03/08 星期日 22:26, Hans Zhang 写道:
> 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@163.com>
> Acked-by: Manivannan Sadhasivam <mani@kernel.org>
> ---
>   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;
>   

Should update the comment of this function, as it states:
"-EINVAL    - Invalid "max-link-speed" property value...
a negative value if the * required property is not found or is invalid."

So it won't validate the speed after this patch. Perhaps it's even
better to note the caller to take the responsiblity to validate it.

> -	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;
>   }
>
Re: [PATCH v7 1/2] PCI: of: Remove max-link-speed generation validation
Posted by Hans Zhang 1 month ago 

On 2026/3/9 08:38, Shawn Lin wrote:
> 在 2026/03/08 星期日 22:26, Hans Zhang 写道:
>> 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@163.com>
>> Acked-by: Manivannan Sadhasivam <mani@kernel.org>
>> ---
>>   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;
> 
> Should update the comment of this function, as it states:
> "-EINVAL    - Invalid "max-link-speed" property value...
> a negative value if the * required property is not found or is invalid."
> 
> So it won't validate the speed after this patch. Perhaps it's even
> better to note the caller to take the responsiblity to validate it.
> 

Hi Shawn,

I plan to modify it as follows. If there are any mistakes, please point 
them out. Thank you very much.

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 9f8eb5df279e..fbb779a94202 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -875,8 +875,9 @@ EXPORT_SYMBOL_GPL(of_pci_supply_present);
   * of_pci_get_max_link_speed - Find the maximum link speed of the 
given device node.
   * @node: Device tree node with the maximum link speed information.
   *
- * This function will try to find the limitation of link speed by finding
- * a property called "max-link-speed" of the given device node.
+ * This function will try to read the "max-link-speed" property of the 
given
+ * device tree node. It does NOT validate the value of the property (e.g.,
+ * range checks for PCIe generations).
   *
   * Return:
   * * > 0	- On success, a maximum link speed.
@@ -889,10 +890,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;
  }


Best regards,
Hans

>> -    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;
>>   }
>>

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.