[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