Re: [PATCH v4 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 2 potential issue(s) to consider:

New issues:
- [High] Valid absolute alias paths matching an overlay fragment name are silently corrupted in dup_and_fixup_symbol_prop().

Pre-existing issues:
- [High] Double free on add_changeset_property() error path for newly created nodes.
--

--- Patch [5]: [PATCH v4 5/6] of/overlay: rewrite /aliases path values to live-tree paths ---
commit b11eb3214c77837761c517184661f83716f7e731
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

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

The commit rewrites overlay-internal alias values in /aliases to their
live-tree paths using dup_and_fixup_symbol_prop(). This ensures that
alias paths are resolvable after the overlay is applied.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index ad4e504825157..2a37b5260ba7a 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -356,6 +356,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]
Can valid absolute alias paths be silently corrupted if they happen to match an
overlay fragment name?

When a user defines an absolute live-tree path in /aliases (e.g.,
serial0 = "/soc/serial_port@1234"), the path starts with "/soc".
dup_and_fixup_symbol_prop() calls __of_find_node_by_path(), which stops
matching at the first '/' delimiter.

If the overlay contains a fragment whose name matches this first component
(e.g. a hand-named fragment "soc"), __of_find_node_by_path() returns that
fragment node. Since the fragment contains an __overlay__ child, the function
assumes it's an overlay-internal path and computes an overlay_name_len
(e.g., 16 for "/soc/__overlay__"). It then blindly slices the string at that
offset, yielding "port@1234", and concatenates it with the fragment's
target_path.

Could this transform a valid absolute live-tree alias into a corrupted path?

> +		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);
>  	}

[Severity: High]
This is a pre-existing issue, but is there a double free on this error path
for newly created nodes?

Looking at the code just below this in 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);
	}
    
	[ ... ]

	if (ret)
		__of_prop_free(new_prop);

If the target node is newly created (!target->in_livetree), new_prop is
prepended to target->np->deadprops. If of_changeset_add_property() fails
(e.g. -ENOMEM), __of_prop_free() is called here.

However, new_prop remains linked in the node's deadprops list. When the
error propagates, the overlay application aborts and rolls back. The rollback
destroys the target node attachment, and of_node_release() calls
property_list_free() on the node's deadprops list, freeing new_prop a second
time.

Will this result in a double-free during node destruction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-nh-of-alias-overlay-v4-0-8ad097e31e36@nexthop.ai?part=5