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

[sashiko-bot@kernel.org]

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