Re: [PATCH 2/4] software node: use kobject_get_unless_zero() in software_node_find_by_name()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] software node: use-after-free on swnode->node in software_node_find_by_name()
- [Low] software node: wrong Fixes tag for software_node_find_by_name()
--

commit e919f34826aef89486e543d4a209837a0357d8a1
Author: Peng Fan <peng.fan@nxp.com>

software node: use kobject_get_unless_zero() in software_node_find_by_name()

This commit updates software_node_find_by_name() to use
kobject_get_unless_zero() instead of swnode_get() so that it skips entries in
swnode_kset->list whose reference count has already dropped to zero.

>     software node: use kobject_get_unless_zero() in software_node_find_by_name()
>     
>     software_node_find_by_name() also walks the swnode kset list and takes a
>     reference with swnode_get(). Like the pointer-based lookup, it can
>     encounter an entry whose refcount has already reached zero but which is
>     still linked until its delayed release callback runs.
>     
>     Use kobject_get_unless_zero() and continue scanning when the entry is
>     already dying.
>     
>     Fixes: 80488a6b1d3c ("software node: Add support for static node descriptors")
>     Signed-off-by: Peng Fan <peng.fan@nxp.com>

[Severity: Low]
Commit 80488a6b1d3c ("software node: Add support for static node
descriptors") introduced software_node_to_swnode(), while
software_node_find_by_name() was added later. Should the Fixes tag instead
reference the commit that introduced software_node_find_by_name()?

  Fixes: 1666faedb567 ("software node: Add software_node_find_by_name()")

> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 19a4153f3fac1..9095d50ba146f 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -831,8 +831,8 @@ software_node_find_by_name(const struct software_node *parent, const char *name)
>  		swnode = kobj_to_swnode(k);
>  		if (parent == swnode->node->parent && swnode->node->name &&
>  		    !strcmp(name, swnode->node->name)) {
> -			swnode_get(swnode);
> -			break;
> +			if (kobject_get_unless_zero(k))
> +				break;
>  		}

[Severity: High]
Can dereferencing swnode->node->parent and swnode->node->name before calling
kobject_get_unless_zero(k) cause a use-after-free when scanning past a dying
swnode?

When a software node is registered via software_node_register(),
software_node_register_node_group(), or device_add_software_node(),
swnode->allocated is 0 and swnode->node points directly to caller-owned
memory rather than an internal copy freed in software_node_release():

drivers/base/swnode.c:software_node_release() {
    ...
	if (swnode->allocated)
		software_node_free(swnode->node);
    ...
}

Once software_node_unregister() drops swnode->kobj.kref to zero and returns,
the caller can immediately free or unmap the backing struct software_node
while the dying swnode remains on swnode_kset->list awaiting kobject cleanup
(for example when CONFIG_DEBUG_KOBJECT_RELEASE delays release, or before
kset_leave() acquires swnode_kset->list_lock).

For instance, ipu_bridge_init() in drivers/media/pci/intel/ipu-bridge.c
unregisters a heap-allocated node and immediately frees it:

err_unregister_ipu:
	software_node_unregister(&bridge->ipu_hid_node);
err_free_bridge:
	kfree(bridge);

Similarly, fwnode_pointer() in lib/tests/printf_kunit.c registers
stack-allocated software_node structures and unregisters them right before
returning from the stack frame.

Should software_node_find_by_name() acquire the reference via
kobject_get_unless_zero(k) before dereferencing swnode->node, and drop that
reference if the parent or name does not match?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-swnode-race-v1-v1-0-9406a67e25be@nxp.com?part=2