Re: [PATCH v8 3/6] of: hold a reference on of_aliases during alias path resolution

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] of: lockless TOCTOU UAF reading of_aliases against concurrent overlay detach
--

commit 66550105e7f11866d2d8f8f454161df1bb855fa8
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of: hold a reference on of_aliases during alias path resolution

This commit takes a reference across the walk of the of_aliases property
list in of_find_node_opts_by_path() to prevent accessing stale views. It
also validates the alias value before resolving it to avoid crashes on
malformed boot FDTs.

>     The pointer load itself needs no locking:
>     of_aliases always holds a reference on the node it points to, so a
>     reader that observes a non-NULL pointer observes a live node. A later
>     patch in this series clears of_aliases and drops that reference when
>     the node is detached at runtime; the detach path holds its own
>     references across the transition.

[Severity: High]
Is this description accurate regarding the lack of locking for the pointer
load?

While the detach path holds a reference during the transition, if
of_changeset_destroy() runs immediately after the revert without any RCU
grace period, it could drop the final reference to the node.

Since of_node_release() uses kfree() and not kfree_rcu(), the object is
destroyed immediately, which leaves a race window open.

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 57313055d9ede..6a6d9f8439dd1 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
[ ... ]
> @@ -1010,16 +1012,21 @@ struct device_node *of_find_node_opts_by_path(const char *path, const char **opt
>  			p = separator;
>  		len = p - path;
>  
> -		/* of_aliases must not be NULL */
> -		if (!of_aliases)
> +		aliases = of_node_get(of_aliases);

[Severity: High]
Can this lockless read lead to a TOCTOU race and a use-after-free?

If a thread calls of_find_node_opts_by_path() and reads the global of_aliases
pointer, but gets preempted before taking the reference in of_node_get():

Thread 1
of_find_node_opts_by_path()
   aliases = of_aliases;
   <preempted>

Thread 2 could concurrently remove an overlay containing a dynamically
created /aliases node. The detach notifier clears of_aliases = NULL and
drops its reference, and then of_changeset_destroy() drops the final
reference to the node, freeing it synchronously via kfree().

When Thread 1 resumes:

Thread 1
   of_node_get(aliases);

Could this lead to dereferencing node->kobj on freed memory?

Should this read be protected by devtree_lock or similar synchronization to
prevent the node from being freed before the reference is taken?

> +		if (!aliases)
>  			return NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-nh-of-alias-overlay-v8-0-0f74f05ac762@nexthop.ai?part=3