Re: [PATCH v8 5/6] of/overlay: rewrite /aliases path values to live-tree paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] of: refcount underflow in dup_and_fixup_symbol_prop()
--

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

of/overlay: rewrite /aliases path values to live-tree paths

This commit rewrites overlay-internal path values in the /aliases node
to live-tree paths so they can be properly resolved. It uses the existing
dup_and_fixup_symbol_prop() helper to accomplish this.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c

[ ... ]

> @@ -370,6 +370,19 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>  		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
>  		if (IS_ERR(new_prop))
>  			return PTR_ERR(new_prop);
> +	} else if (!is_pseudo_property(overlay_prop->name) &&
> +		   of_node_is_aliases(target->np)) {
> +		/* rewrite overlay-internal alias values to live-tree paths */
> +		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);

[Severity: High]
Could this call to dup_and_fixup_symbol_prop() trigger a reference count
underflow?

Looking at dup_and_fixup_symbol_prop(), it retrieves nodes using
__of_find_node_by_path() but then explicitly drops references to them:

drivers/of/overlay.c:dup_and_fixup_symbol_prop() {
    ...
	fragment_node = __of_find_node_by_path(ovcs->overlay_root, path + 1);
	overlay_node = __of_find_node_by_path(fragment_node, "__overlay__/");
	of_node_put(fragment_node);
	of_node_put(overlay_node);
    ...
}

However, as documented in drivers/of/of_private.h, __of_find_node_by_path()
operates without taking node references:

drivers/of/of_private.h:
/*
 * All functions with two leading underscores operate
 * without taking node references, so you either have to
 * own the devtree lock or work on detached trees only.
 */
struct device_node *__of_find_node_by_path(const struct device_node *parent, ...);

Because the references were never initially acquired, calling of_node_put()
here will underflow the reference count. This drops the independent references
held by the changeset fragments and could lead to premature freeing of the
nodes and a use-after-free during subsequent access.

> +		if (new_prop == ERR_PTR(-ENOMEM))
> +			return -ENOMEM;
> +		if (IS_ERR(new_prop)) {
> +			if (new_prop == ERR_PTR(-EINVAL))
> +				pr_warn("%pOF/%s is not a valid string; alias will be inert\n",
> +					target->np, overlay_prop->name);
> +			/* not overlay-internal: copy verbatim, consumers validate */
> +			new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
> +		}
>  	} else {
>  		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>  	}

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