[PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[Alban Bedel]

fwnode_property_get_reference_args() should return -ENOENT when an out
of bound index is passed. An issue arised with the OF backend because
the OF API use signed indexes while the fwnode API use unsigned ones.
When an index value greater the INT_MAX was passed to the OF backend
it got casted to a negative value and it returned -EINVAL instead of
-ENOENT. This patch add a check to of_fwnode_get_reference_args() to
catch negative index before they are passed to the OF API and return
-ENOENT right away.

This issue appeared when the following pattern was used in the LED
subsystem:

    index = fwnode_property_match_string(fwnode, "led-names", name)
    led_node = fwnode_find_reference(fwnode, "leds", index);

Unlike the same pattern with the OF API, this pattern implicitly cast
the signed return value of fwnode_property_match_string() to an
unsigned index leading to the above issue with the OF backend. It can
be argued that the return value of fwnode_property_match_string()
should be checked separately, but I think there is value in supporting
such simple and straight to the point patterns.

Link: https://lore.kernel.org/linux-leds/aimVRwJPhlGxsIUj@tom-desktop/T/#mc43cbf7e0599991b56dd0d9680714d28d145fbc8
Cc: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
Signed-off-by: Alban Bedel <alban.bedel@lht.dlh.de>
---
 drivers/of/property.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 136946f8b746f..eace2d1847b99 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1157,6 +1157,13 @@ of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 	unsigned int i;
 	int ret;
 
+	/* This function should return -ENOENT for out of bound indexes,
+	 * but the OF API uses signed indexes and consider negative indexes
+	 * as invalid. Catch them here to correctly implement the fwnode API.
+	 */
+	if ((int)index < 0)
+		return -ENOENT;
+
 	if (nargs_prop)
 		ret = of_parse_phandle_with_args(to_of_node(fwnode), prop,
 						 nargs_prop, index, &of_args);
-- 
2.39.5

Re: [PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[sashiko-bot]

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

Pre-existing issues:
- [High] Integer overflow in the bounds check of `software_node_get_reference_args` allows out-of-bounds memory read and potential kernel panic.
--

commit c268eb898558d8f55476cb8562a12b6fa754123b
Author: Alban Bedel <alban.bedel@lht.dlh.de>

of: property: Fix of_fwnode_get_reference_args() with negative index

This commit catches negative index values that are implicitly cast to
unsigned in the fwnode API before they are passed to the OF backend.
It ensures out-of-bounds indices return -ENOENT instead of -EINVAL.

Link: https://lore.kernel.org/linux-leds/aimVRwJPhlGxsIUj@tom-desktop/T/#mc43cbf7e0599991b56dd0d9680714d28d145fbc8

> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 136946f8b746f..eace2d1847b99 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1157,6 +1157,13 @@ of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
>  	unsigned int i;
>  	int ret;
>  
> +	/* This function should return -ENOENT for out of bound indexes,
> +	 * but the OF API uses signed indexes and consider negative indexes
> +	 * as invalid. Catch them here to correctly implement the fwnode API.
> +	 */
> +	if ((int)index < 0)
> +		return -ENOENT;

[Severity: High]
This is a pre-existing issue, but does this exact same implicit cast pattern
cause a regression in the software node backend?

Looking at software_node_get_reference_args() in drivers/base/swnode.c:

drivers/base/swnode.c:software_node_get_reference_args() {
    ...
    if ((index + 1) * sizeof(*ref) > prop->length)
        return -ENOENT;

    ref_array = prop->pointer;
    ref = &ref_array[index];
    ...
}

If index is 0xffffffff (from a -1 return value implicitly cast to unsigned),
index + 1 wraps around to 0. The check 0 * sizeof(*ref) evaluates as not
greater than prop->length, completely bypassing the bounds check.

Could this lead to an out-of-bounds array access when computing
&ref_array[index], causing a panic when dereferenced?

Should a similar check for negative indices be added to the swnode backend?

> +
>  	if (nargs_prop)
>  		ret = of_parse_phandle_with_args(to_of_node(fwnode), prop,
>  						 nargs_prop, index, &of_args);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611102806.2643869-1-alban.bedel@lht.dlh.de?part=1

Re: [PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[Krzysztof Kozlowski]

On Thu, Jun 11, 2026 at 12:28:06PM +0200, Alban Bedel wrote:
> fwnode_property_get_reference_args() should return -ENOENT when an out
> of bound index is passed. An issue arised with the OF backend because
> the OF API use signed indexes while the fwnode API use unsigned ones.
> When an index value greater the INT_MAX was passed to the OF backend
> it got casted to a negative value and it returned -EINVAL instead of

INT_MAX is not out of bound for this function. It is invalid value,
because OF code expects signed.
 
> -ENOENT. This patch add a check to of_fwnode_get_reference_args() to
> catch negative index before they are passed to the OF API and return
> -ENOENT right away.

I do not understand why are you fixing this issue that way. For this
API, the INT_MAX is correct value, but you claim that it is wrong and
should be ENOENT (even if there is entry).

Fine, if this is not a correct value, then EINVAL.

But more important I think this should be just fixed in different way -
why index in OF calls is signed in the first place? All indices are
supposed to be unsigned in general, because that is both logical and
readable when accessing arrays.

> 
> This issue appeared when the following pattern was used in the LED
> subsystem:
> 
>     index = fwnode_property_match_string(fwnode, "led-names", name)
>     led_node = fwnode_find_reference(fwnode, "leds", index);
> 
> Unlike the same pattern with the OF API, this pattern implicitly cast
> the signed return value of fwnode_property_match_string() to an
> unsigned index leading to the above issue with the OF backend. It can
> be argued that the return value of fwnode_property_match_string()
> should be checked separately, but I think there is value in supporting
> such simple and straight to the point patterns.
> 
> Link: https://lore.kernel.org/linux-leds/aimVRwJPhlGxsIUj@tom-desktop/T/#mc43cbf7e0599991b56dd0d9680714d28d145fbc8
> Cc: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>
> Signed-off-by: Alban Bedel <alban.bedel@lht.dlh.de>
> ---
>  drivers/of/property.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 136946f8b746f..eace2d1847b99 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1157,6 +1157,13 @@ of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
>  	unsigned int i;
>  	int ret;
>  
> +	/* This function should return -ENOENT for out of bound indexes,

/*

Please use Linux coding style comments.

> +	 * but the OF API uses signed indexes and consider negative indexes
> +	 * as invalid. Catch them here to correctly implement the fwnode API.

Best regards,
Krzysztof

Re: [PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[Alban Bedel]

On Mon, 15 Jun 2026 08:33:32 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On Thu, Jun 11, 2026 at 12:28:06PM +0200, Alban Bedel wrote:
> > fwnode_property_get_reference_args() should return -ENOENT when an out
> > of bound index is passed. An issue arised with the OF backend because
> > the OF API use signed indexes while the fwnode API use unsigned ones.
> > When an index value greater the INT_MAX was passed to the OF backend
> > it got casted to a negative value and it returned -EINVAL instead of  
> 
> INT_MAX is not out of bound for this function. It is invalid value,
> because OF code expects signed.

But this is fwnode code, it use the OF API but it should implement the
fwnode API which, unlike the OF API, use unsigned index.

> > -ENOENT. This patch add a check to of_fwnode_get_reference_args() to
> > catch negative index before they are passed to the OF API and return
> > -ENOENT right away.  
> 
> I do not understand why are you fixing this issue that way. For this
> API, the INT_MAX is correct value, but you claim that it is wrong and
> should be ENOENT (even if there is entry).
>
> Fine, if this is not a correct value, then EINVAL.

Indices larger than INT_MAX are valid in the fwnode API, so returning
-EINVAL is not appropriate here.

> But more important I think this should be just fixed in different way -
> why index in OF calls is signed in the first place? All indices are
> supposed to be unsigned in general, because that is both logical and
> readable when accessing arrays.
 
I agree that would be the better fix in the long run. But that would
impact a lot more code and I found it difficult to ensure it would not
potentially break some of its users. 

Alban

Re: [PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[Krzysztof Kozlowski]

On 15/06/2026 11:33, Alban Bedel wrote:
> On Mon, 15 Jun 2026 08:33:32 +0200
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
>> On Thu, Jun 11, 2026 at 12:28:06PM +0200, Alban Bedel wrote:
>>> fwnode_property_get_reference_args() should return -ENOENT when an out
>>> of bound index is passed. An issue arised with the OF backend because
>>> the OF API use signed indexes while the fwnode API use unsigned ones.
>>> When an index value greater the INT_MAX was passed to the OF backend
>>> it got casted to a negative value and it returned -EINVAL instead of  
>>
>> INT_MAX is not out of bound for this function. It is invalid value,
>> because OF code expects signed.
> 
> But this is fwnode code, it use the OF API but it should implement the
> fwnode API which, unlike the OF API, use unsigned index.
>>> -ENOENT. This patch add a check to of_fwnode_get_reference_args() to
>>> catch negative index before they are passed to the OF API and return
>>> -ENOENT right away.  
>>
>> I do not understand why are you fixing this issue that way. For this
>> API, the INT_MAX is correct value, but you claim that it is wrong and
>> should be ENOENT (even if there is entry).
>>
>> Fine, if this is not a correct value, then EINVAL.
> 
> Indices larger than INT_MAX are valid in the fwnode API, so returning
> -EINVAL is not appropriate here.
> 

Then neither ENOENT are.

But really, EINVAL is correct here. This is OF implementation, so this
implementation decides what is EINVAL and what is right. Not fwnode API.


>> But more important I think this should be just fixed in different way -
>> why index in OF calls is signed in the first place? All indices are
>> supposed to be unsigned in general, because that is both logical and
>> readable when accessing arrays.
>  
> I agree that would be the better fix in the long run. But that would
> impact a lot more code and I found it difficult to ensure it would not
> potentially break some of its users. 

I don't see how this fixes anything. You basically replace correct
return value to a different one.

Best regards,
Krzysztof

Re: [PATCH] of: property: Fix of_fwnode_get_reference_args() with negative index

[Alban Bedel]

On Mon, 15 Jun 2026 11:54:01 +0200
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 15/06/2026 11:33, Alban Bedel wrote:
> > On Mon, 15 Jun 2026 08:33:32 +0200
> > Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >   
> >> On Thu, Jun 11, 2026 at 12:28:06PM +0200, Alban Bedel wrote:  
> >>> fwnode_property_get_reference_args() should return -ENOENT when an out
> >>> of bound index is passed. An issue arised with the OF backend because
> >>> the OF API use signed indexes while the fwnode API use unsigned ones.
> >>> When an index value greater the INT_MAX was passed to the OF backend
> >>> it got casted to a negative value and it returned -EINVAL instead of    
> >>
> >> INT_MAX is not out of bound for this function. It is invalid value,
> >> because OF code expects signed.  
> > 
> > But this is fwnode code, it use the OF API but it should implement the
> > fwnode API which, unlike the OF API, use unsigned index.  
> >>> -ENOENT. This patch add a check to of_fwnode_get_reference_args() to
> >>> catch negative index before they are passed to the OF API and return
> >>> -ENOENT right away.    
> >>
> >> I do not understand why are you fixing this issue that way. For this
> >> API, the INT_MAX is correct value, but you claim that it is wrong and
> >> should be ENOENT (even if there is entry).
> >>
> >> Fine, if this is not a correct value, then EINVAL.  
> > 
> > Indices larger than INT_MAX are valid in the fwnode API, so returning
> > -EINVAL is not appropriate here.
> >   
> 
> Then neither ENOENT are.
>
> But really, EINVAL is correct here. This is OF implementation, so this
> implementation decides what is EINVAL and what is right. Not fwnode API.

I think there is a missunderstanding here. The function we are talking
about, of_fwnode_get_reference_args(), is the OF backed implementation
of fwnode_property_get_reference_args(). As such it must follow the API
documented by fwnode_property_get_reference_args() which list the
following return values:

 * Return: %0 on success
 *	    %-ENOENT when the index is out of bounds, the index has an empty
 *		     reference or the property was not found
 *	    %-EINVAL on parse error
 *	    %-ENOTCONN when the remote firmware node exists but has not been
 *		       registered yet

It is not explicitly documented what should be returned if the index is
not representable in the backend, but considering it out of bound seems
to be the most sensible thing to do.

Alban