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

[sashiko-bot@kernel.org]

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