Re: [PATCH v3 3/4] of/overlay: rewrite /aliases path values to live-tree paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Pseudo-properties of newly created /aliases nodes are incorrectly validated as C string paths, breaking valid overlays.

Pre-existing issues:
- [High] Use-After-Free and Double Free in add_changeset_property error path.
- [Medium] Memory leak of duplicated device node on error path in add_changeset_node().
--

--- Patch [3]: [PATCH v3 3/4] of/overlay: rewrite /aliases path values to live-tree paths ---
commit a80ad646f2d2ec17e2e79fd9043171b15218e86b
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

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

This commit rewrites overlay-internal path strings for /aliases entries to
live-tree paths, similar to /__symbols__. It ensures that /aliases values
correctly reference nodes in the live tree.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index b6545905cf67d..9f85278c577ac 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -350,6 +350,21 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>  		if (prop)
>  			return -EINVAL;
>  		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
> +	} else if (target->np->parent &&
> +		   of_node_is_root(target->np->parent) &&
> +		   of_node_name_eq(target->np, "aliases")) {
> +		/* /aliases values are deref'd as C strings; reject malformed input */
> +		if (!overlay_prop->value || overlay_prop->length < 2 ||
> +		    strnlen(overlay_prop->value, overlay_prop->length) >=
> +			overlay_prop->length)
> +			return -EINVAL;

[Severity: Medium]
Does this code incorrectly reject pseudo-properties on newly created /aliases
nodes?

Since the is_pseudo_property() filter at the top of add_changeset_property()
only applies when target->in_livetree is true, pseudo-properties (like
"phandle") on a newly created /aliases node will fall through to this logic.

A "phandle" is a 32-bit integer, so it may lack a null byte. This causes
strnlen() >= length to evaluate to true, falsely returning -EINVAL and aborting
valid overlay applications.

> +
> +		/* rewrite overlay-internal paths to live-tree paths */
> +		if (!strncmp(overlay_prop->value, "/fragment@",
> +			     strlen("/fragment@")))
> +			new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);
> +		else
> +			new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>  	} else {
>  		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
add_changeset_property() cause a use-after-free or double-free?

If of_changeset_add_property() fails later in this function, we do this:

drivers/of/overlay.c:add_changeset_property() {
    ...
	if (!prop) {
		if (!target->in_livetree) {
			new_prop->next = target->np->deadprops;
			target->np->deadprops = new_prop;
		}
		ret = of_changeset_add_property(&ovcs->cset, target->np,
						new_prop);
	} else {
    ...
	if (ret)
		__of_prop_free(new_prop);
	return ret;
}

Since new_prop is freed but left linked in target->np->deadprops, won't
of_node_release() later iterate over deadprops and double-free this same
property?


[Severity: Medium]
This isn't a bug introduced by this patch, but does the error path in
add_changeset_node() permanently leak the duplicated node tchild?

When attaching a newly duplicated child node fails, the function returns
immediately without freeing the node:

drivers/of/overlay.c:add_changeset_node() {
    ...
	if (!tchild) {
		tchild = __of_node_dup(NULL, node_kbasename);
        ...
		ret = of_changeset_attach_node(&ovcs->cset, tchild);
		if (ret)
			return ret;
    ...
}

Should there be an of_node_put(tchild) before returning the error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-nh-of-alias-overlay-v3-0-7001028fe2f5@nexthop.ai?part=3