Re: [PATCH 1/4] software node: do not resurrect swnode being released

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] software node: transient ref in software_node_to_swnode() risks UAF
- [Medium] software node: re-registering named or parent node hits -EEXIST
--

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

software node: do not resurrect swnode being released

This commit updates software node lookup in drivers/base/swnode.c to use
kobject_get_unless_zero() so that zero-reference swnodes awaiting release on
swnode_kset->list are skipped rather than resurrected. It introduces
software_node_to_swnode_get() and software_node_register_node() for callers
that acquire a reference, while keeping software_node_to_swnode() as a
wrapper that drops the reference before returning.

>     software node: do not resurrect swnode being released
>     
>     software_node_to_swnode() walks the kset list under list_lock, but the
>     last kobject_put() drops the reference count to zero before
>     kobject_cleanup() removes the object from the list. A lookup can
>     therefore find a zero-ref swnode and call kobject_get() on it, which
>     triggers refcount warnings and a use-after-free.
>     
>     One way to hit this is the dwc3-imx driver on i.MX95: when
>     dwc3_core_probe() returns -EPROBE_DEFER, dwc3_imx_probe() removes the
>     static software node from the device, and the deferred probe retry
>     immediately adds the same node again. The previous swnode is still
>     waiting for its release callback, and CONFIG_DEBUG_KOBJECT_RELEASE makes
>     the window large and deterministic.
>     
>     Fix the lookup by acquiring references with kobject_get_unless_zero()
>     and skipping objects that are already in their release path. Use a
>     reference-returning variant for callers that keep the node (register,
>     unregister and device_add_software_node()) and keep a borrowed-pointer
>     variant for software_node_fwnode().
>     
>     To avoid looking the node up again after registration,
>     software_node_register() now uses an internal helper that returns the
>     newly created swnode reference, and device_add_software_node() consumes
>     that reference directly.
>     
>     Fixes: 80488a6b1d3c ("software node: Add support for static node descriptors")
>     Signed-off-by: Peng Fan <peng.fan@nxp.com>
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index 84a77ffa3dc4b..19a4153f3fac1 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c

[ ... ]

> @@ -98,6 +124,21 @@ software_node_to_swnode(const struct software_node *node)
>  	return swnode;
>  }
>  
> +/*
> + * Borrowed-pointer variant of software_node_to_swnode_get().  The caller must
> + * guarantee the lifetime of @node by other means.
> + */
> +static struct swnode *
> +software_node_to_swnode(const struct software_node *node)
> +{
> +	struct swnode *swnode = software_node_to_swnode_get(node);
> +
> +	if (swnode)
> +		swnode_put(swnode);
> +
> +	return swnode;
> +}

[Severity: High]
Can calling swnode_put(swnode) here drop the final reference and free swnode
before returning it?

In software_node_to_swnode(), software_node_to_swnode_get(node) acquires
swnode_kset->list_lock, increments candidate->kobj.kref (for example from 1
to 2) via kobject_get_unless_zero(), and releases list_lock.

If the owner of the swnode drops its final reference (via
device_remove_software_node(), software_node_unregister(), or
fwnode_remove_software_node()) before software_node_to_swnode() runs
swnode_put(swnode), the owner's kobject_put() only decrements kref from 2 to
1 instead of releasing the object:

1. If the owner immediately re-registers or re-adds the software_node during
   that window, software_node_to_swnode_get() sees the dying swnode with
   kref == 1 and either fails software_node_register() with -EEXIST or
   reattaches the dying swnode in device_add_software_node().

2. When software_node_to_swnode() then calls swnode_put(swnode), it drops
   kref from 1 to 0, invoking kobject_release() -> kobject_cleanup() ->
   software_node_release() -> kfree(swnode), and returns a dangling swnode
   pointer to its caller.

This is reachable through software_node_fwnode() when resolving referenced
supplier or remote nodes (whose lifetimes are not pinned by the caller) in
software_node_get_reference_args(), software_node_graph_get_remote_endpoint(),
and software_node_add_links(), as well as through software_node_unregister():

drivers/base/swnode.c:software_node_unregister() {
	struct swnode *swnode;

	swnode = software_node_to_swnode(node);
	if (swnode)
		fwnode_remove_software_node(&swnode->fwnode);
}

Also, the commit message mentions using the reference-returning variant for
unregister ("Use a reference-returning variant for callers that keep the node
(register, unregister and device_add_software_node())"), but
software_node_unregister() still calls software_node_to_swnode(node).

[ ... ]

> @@ -977,21 +1006,39 @@ void software_node_unregister_node_group(const struct software_node * const *nod
>  }
>  EXPORT_SYMBOL_GPL(software_node_unregister_node_group);
>  
> +static struct swnode *
> +software_node_register_node(const struct software_node *node)
> +{
> +	struct swnode *parent, *swnode;
> +	struct fwnode_handle *fwnode;
> +
> +	swnode = software_node_to_swnode_get(node);
> +	if (swnode) {
> +		swnode_put(swnode);
> +		return ERR_PTR(-EEXIST);
> +	}
> +
> +	parent = software_node_to_swnode_get(node->parent);
> +	if (node->parent && !parent)
> +		return ERR_PTR(-EINVAL);
> +
> +	fwnode = swnode_register(node, parent, 0);

[Severity: Medium]
What happens if a named static software_node (node->name != NULL) or a
parent/child software_node group is removed on -EPROBE_DEFER and
re-registered before kobject_cleanup() runs?

When the last reference to an swnode is dropped via swnode_put() ->
kobject_put(), __kobject_del() (which removes the sysfs directory) and
kobject_put(parent) only run inside kobject_cleanup(), which is delayed by
1-4 seconds on a workqueue when CONFIG_DEBUG_KOBJECT_RELEASE=y.

For unnamed nodes (node->name == NULL, such as dwc3_imx_swnode),
swnode_register() allocates a fresh IDA ID and creates "node1" while "node0"
is still waiting to be cleaned up. However, when node->name is set:

drivers/base/swnode.c:swnode_register() {
	...
	if (node->name)
		ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
					   parent ? &parent->kobj : NULL,
					   "%s", node->name);
}

kobject_init_and_add() attempts to create a sysfs directory with the same
name as the dying swnode that is still present in sysfs, failing in
sysfs_create_dir_ns() with -EEXIST and a sysfs_warn_dup() warning.

Similarly, when a node group is unregistered via
software_node_unregister_node_group(), the dying child kobject holds its
reference on parent->kobj until kobject_cleanup() finishes; the parent's
refcount therefore stays non-zero during the delayed release window, and
re-registering the parent in software_node_register_node() still fails with
-EEXIST.

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