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

New issues:
- [Medium] A structural validation failure in `dup_and_fixup_symbol_prop()` is incorrectly masked as a memory allocation failure.

Pre-existing issues:
- [High] The `__of_prop_dup` fallback permits non-null-terminated alias strings to enter the live tree, causing a Slab Out-Of-Bounds Read.
- [High] Double Free / Use-After-Free of `new_prop` on memory allocation failure.
- [High] Memory leak of successfully processed device nodes on early exit in `init_overlay_changeset()`.
--

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

The commit rewrites /aliases entries added by an overlay to use live-tree
paths instead of overlay-internal fragment paths. It checks if the alias
value starts with /fragment@ and uses dup_and_fixup_symbol_prop() to fix
it up. Other alias values fall through to __of_prop_dup().

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 654a70d5cb075..69358811a7f94 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -350,6 +350,33 @@ 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") &&
> +		   overlay_prop->length >= sizeof("/fragment@") &&
> +		   overlay_prop->value &&
> +		   strnlen(overlay_prop->value, overlay_prop->length) <
> +			overlay_prop->length &&
> +		   !strncmp(overlay_prop->value, "/fragment@",
> +			    strlen("/fragment@"))) {

[Severity: High]
This isn't a bug introduced by this patch, but does this logic still allow
non-null-terminated strings to enter the live tree?

If an overlay provides an /aliases property with a non-null-terminated
string that does not start with /fragment@ (e.g., ttyS0 missing a null byte),
the else if condition evaluates to false.

Execution falls through to the else block:

drivers/of/overlay.c:add_changeset_property() {
    ...
	} else {
		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
	}
    ...
}

Here __of_prop_dup() blindly copies the non-null-terminated bytes directly
into the live device tree. When a caller such as of_find_node_opts_by_path()
attempts to resolve the alias, it passes pp->value to of_find_node_by_path(),
which uses standard C string functions (strncmp, strchr) on the buffer,
reading past the end.

> +		/*
> +		 * /aliases property values that reference a labeled node
> +		 * inside this overlay are rendered by dtc as string paths
> +		 * "/fragment@N/__overlay__/..." — the overlay's internal
> +		 * layout rather than where the node lives after apply.
> +		 * Reuse dup_and_fixup_symbol_prop() (which already handles
> +		 * this rewrite for /__symbols__) so of_alias_get_id() can
> +		 * resolve the value in the live tree.
> +		 *
> +		 * The property-value shape (non-empty, null-terminated
> +		 * within pp->length, prefix "/fragment@") is validated
> +		 * above so a NULL return from dup_and_fixup_symbol_prop()
> +		 * here means -ENOMEM, not "not our shape"; let the shared
> +		 * -ENOMEM check below handle it rather than falling back
> +		 * to a raw dup that would inject an unresolvable path
> +		 * into the live tree.
> +		 */
> +		new_prop = dup_and_fixup_symbol_prop(ovcs, overlay_prop);

[Severity: Medium]
Does this incorrectly mask a structural validation failure as a memory
allocation failure?

When an overlay provides a structurally valid /aliases property that starts
with /fragment@ and is null-terminated, but references a non-existent
overlay fragment, dup_and_fixup_symbol_prop() will legitimately return NULL:

drivers/of/overlay.c:dup_and_fixup_symbol_prop() {
    ...
	if (k >= ovcs->count)
		return NULL;
    ...
}

The shared error handler below then incorrectly converts this structural
failure into a memory exhaustion error (-ENOMEM).

>  	} else {
>  		new_prop = __of_prop_dup(overlay_prop, GFP_KERNEL);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a double free
of new_prop on memory allocation failure?

If !target->in_livetree, new_prop is linked into target->np->deadprops:

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

If of_changeset_add_property() returns an error, the function explicitly
calls __of_prop_free(new_prop):

drivers/of/overlay.c:add_changeset_property() {
    ...
	if (ret)
		__of_prop_free(new_prop);
	return ret;
}

Since new_prop was not unlinked from the deadprops list, when the overlay
application aborts and of_node_release() iterates over deadprops, will it
call __of_prop_free() again on the already-freed pointer?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a memory leak of
successfully processed device nodes on early exit in
init_overlay_changeset()?

When applying a multi-fragment device tree overlay, if an intermediate
fragment fails find_target() (e.g., due to an invalid target-path), the
code jumps to err_out:

drivers/of/overlay.c:init_overlay_changeset() {
    ...
		fragment->target = find_target(node, target_base);
		if (!fragment->target) {
			of_node_put(fragment->overlay);
			ret = -EINVAL;
			of_node_put(node);
			goto err_out;
		}
    ...
}

As the function iterates over the fragments, it increments the reference
counts for both the overlay and target device nodes, storing them in the
fragments array. When find_target() fails for a subsequent fragment, the
function exits with an error without updating ovcs->count (which remains 0).

The cleanup function free_overlay_changeset() relies on ovcs->count to loop
through and release the node references. Because the count is 0, the cleanup
loop is skipped entirely, causing a permanent leak of the successfully
processed node references from the prior fragments.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-nh-of-alias-overlay-v1-0-27da6848dd84@nexthop.ai?part=3