[PATCH v2 1/2] of: property: Fix of_fwnode_get_reference_args() with negative index

[PATCH v2 1/2] 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Alban Bedel <alban.bedel@lht.dlh.de>
---
v2: Fixed comment block to follow linux coding style.
---
 drivers/of/property.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 136946f8b746f..a4e9ffe1a4121 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1157,6 +1157,14 @@ 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

[PATCH v2 2/2] software node: Fix software_node_get_reference_args() with index -1

[Alban Bedel]

The bounds check for the index passed to
software_node_get_reference_args() was failing when passed UINT_MAX,
this in turn would lead to an out of bound access in the property
array. Fix the bound check to also cover the UINT_MAX case.

Fixes: 31e4e12e0e960 ("software node: Correct a OOB check in software_node_get_reference_args()")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-devicetree/20260611103904.7CB131F00893@smtp.kernel.org/
Signed-off-by: Alban Bedel <alban.bedel@lht.dlh.de>
--
v2: No changes. Only submit this patch along with the patch that
    triggered the Sashiko report, to hopefully avoid another useless
    report.
---
 drivers/base/swnode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 869228a65cb36..2bc76f01eb77d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -537,7 +537,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
 	if (prop->is_inline)
 		return -EINVAL;
 
-	if ((index + 1) * sizeof(*ref) > prop->length)
+	if (index >= prop->length / sizeof(*ref))
 		return -ENOENT;
 
 	ref_array = prop->pointer;
-- 
2.39.5

Re: [PATCH v2 2/2] software node: Fix software_node_get_reference_args() with index -1

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free vulnerability in software node reference resolution.
- [High] Data race and linked list corruption in software node children management.
--

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

software node: Fix software_node_get_reference_args() with index -1

This patch corrects an out-of-bounds access check in
software_node_get_reference_args() by ensuring the bounds check correctly
handles UINT_MAX as an index.

Closes: https://lore.kernel.org/linux-devicetree/20260611103904.7CB131F00893@smtp.kernel.org/

> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index a19f8f722bc8e..e76f7c36f19e6 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -529,7 +529,7 @@ software_node_get_reference_args(const struct fwnode_handle *fwnode,
>  	if (prop->is_inline)
>  		return -EINVAL;
>  
> -	if ((index + 1) * sizeof(*ref) > prop->length)
> +	if (index >= prop->length / sizeof(*ref))
>  		return -ENOENT;
>  
>  	ref_array = prop->pointer;

[Severity: High]
This isn't a bug introduced by this patch, but it looks like there's a
pre-existing use-after-free risk later in this function.

When we resolve the reference here:

	if (ref->swnode)
		refnode = software_node_fwnode(ref->swnode);
	...
	args->fwnode = fwnode_handle_get(refnode);

software_node_fwnode() calls software_node_to_swnode(), which drops the
swnode_kset->list_lock and returns the swnode pointer without elevating its
reference count via kobject_get():

drivers/base/swnode.c:software_node_to_swnode() {
    ...
    list_for_each_entry(k, &swnode_kset->list, entry) {
        swnode = kobj_to_swnode(k);
        if (swnode->node == node)
            break;
        swnode = NULL;
    }
    spin_unlock(&swnode_kset->list_lock);
    return swnode;
}

Since the pointer is unreferenced, could it be concurrently unregistered
and freed (e.g., by software_node_unregister()) before we call
fwnode_handle_get() on it?

[Severity: High]
This is also a pre-existing issue, but I noticed a potential data race and
linked list corruption in the software node children management in
drivers/base/swnode.c.

In swnode_register(), child nodes are added to the parent's list without
a lock:

drivers/base/swnode.c:swnode_register() {
    ...
    if (parent)
        list_add_tail(&swnode->entry, &parent->children);
    ...
}

And in software_node_release(), they are removed without synchronization:

drivers/base/swnode.c:software_node_release() {
    ...
    if (swnode->parent) {
        ida_free(&swnode->parent->child_ids, swnode->id);
        list_del(&swnode->entry);
    }
    ...
}

If multiple child nodes are concurrently registered or unregistered under the
same parent (e.g., dynamically by drivers calling
fwnode_create_software_node()), could this corrupt the doubly linked list and
crash the kernel?

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