Re: [PATCH v8 4/6] of: update /aliases lookup on reconfig notifications

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] of: UAF in of_find_node_opts_by_path() due to lockless read of of_aliases
--

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

of: update /aliases lookup on reconfig notifications

This commit introduces a reconfig notifier to dynamically update the /aliases
lookup table when device tree overlays add or remove aliases. It mirrors
property changes into aliases_lookup and manages entries added at runtime
versus boot time. An aliases_mutex is added to protect the lookup list.

Link: https://lore.kernel.org/lkml/1435675876-2159-1-git-send-email-geert+renesas@glider.be/ [1]
Link: https://lore.kernel.org/lkml/20150630172131.D4E6CC4041A@trevor.secretlab.ca/ [2]

> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 6a6d9f8439dd1..b70dedce06fe4 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
[ ... ]
> @@ -1934,6 +1934,159 @@ static void of_alias_add(struct alias_prop *ap, struct device_node *np,
>  		 ap->alias, ap->stem, ap->id, np);
>  }
>  
[ ... ]
> +static int of_aliases_reconfig_notifier(struct notifier_block *nb,
> +					unsigned long action, void *arg)
> +{
> +	struct of_reconfig_data *rd = arg;
> +
> +	/* of_aliases may still be NULL when an overlay creates the node */
> +	if (!rd->dn || !of_node_is_aliases(rd->dn))
> +		return NOTIFY_DONE;
> +
> +	mutex_lock(&aliases_mutex);
> +	switch (action) {
> +	case OF_RECONFIG_ATTACH_NODE:
> +		if (!of_aliases)
> +			of_aliases = of_node_get(rd->dn);
> +		break;
> +	case OF_RECONFIG_DETACH_NODE:
> +		if (of_aliases == rd->dn) {
> +			of_aliases = NULL;
> +			of_aliases_forget_all();
> +			of_node_put(rd->dn);
> +		}
> +		break;

[Severity: High]
Does this lockless read of of_aliases in of_find_node_opts_by_path() introduce
a Use-After-Free race?

The commit message states: "on DETACH the changeset holds its own references
across the notifier so the put here cannot be the final one while readers
still walk the node."

While the changeset holds a reference during the notifier, a reader in
of_find_node_opts_by_path() in drivers/of/base.c reading of_aliases is
completely lockless and not in an RCU read-side critical section. If the
reader thread is preempted immediately after loading the of_aliases pointer
but before calling of_node_get():

CPU1 (Reader)
of_find_node_opts_by_path()
   // Starts executing: aliases = of_node_get(of_aliases);
   // Reads global of_aliases pointer
   <preempted>

CPU2 (Changeset detach)
of_aliases_reconfig_notifier()
   of_aliases = NULL;
   of_node_put(rd->dn);
<changeset completes detachment, drops its final reference>
<node is freed>

CPU1 (Reader resumes)
   // Calls of_node_get() with the stale pointer
   of_node_get(stale_pointer); // Use-After-Free

Can the reader be preempted long enough for the changeset detach to fully
complete and free the node before the reader increments the refcount?

> +	case OF_RECONFIG_ADD_PROPERTY:
> +		of_alias_create(rd->prop, alias_alloc, true);
> +		break;

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