Re: [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but lockless traversal of a shared list `ovcs_list` in `of_overlay_remove_all()` leads to a data race and potential Use-After-Free.
--

--- Patch [6]: [PATCH v6 06/10] of/overlay: only treat a positive changeset id as registered ---
commit ff7e3da3be86949a404438d74d2cffe93f4a4cba
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

of/overlay: only treat a positive changeset id as registered

of_overlay_fdt_apply() stores the idr_alloc() return value in
ovcs->id before checking it. On failure the stored id is negative,
free_overlay_changeset()'s "if (ovcs->id)" check passes, idr_remove()
is called with a negative id and list_del() runs on ovcs->ovcs_list,
which is not initialized until after the id allocation. An allocation
failure at that point dereferences NULL.

Make free_overlay_changeset() treat only a strict-positive id as
registered. The rest of the function already copes with a
partially-initialized ovcs, so the error path stays a plain
goto err_free_ovcs.

Cc: stable@vger.kernel.org

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 284c9bc6c9cfc..9b9f198a1d70a 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -860,7 +860,8 @@ static void free_overlay_changeset(struct overlay_changeset *ovcs)
>  	if (ovcs->cset.entries.next)
>  		of_changeset_destroy(&ovcs->cset);
>  
> -	if (ovcs->id) {
> +	/* a failed idr_alloc() leaves its negative error in ovcs->id */
> +	if (ovcs->id > 0) {
>  		idr_remove(&ovcs_idr, ovcs->id);
>  		list_del(&ovcs->ovcs_list);

[Severity: High]
This problem wasn't introduced by this patch, but lockless traversal of a
shared list ovcs_list in of_overlay_remove_all() leads to a data race.

Is it safe to iterate ovcs_list without holding of_mutex?

drivers/of/overlay.c:of_overlay_remove_all() {
	...
	list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
		ret = of_overlay_remove(&ovcs->id);
	...
}

The list_for_each_entry_safe_reverse() iterator only protects against
the deletion of the current entry being processed.

If another thread concurrently modifies the list (e.g., via
of_overlay_fdt_apply() or of_overlay_remove()), could this corrupt the
list pointers or remove the cached next entry, leading to a Use-After-Free?

>  		ovcs->id = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-nh-of-alias-overlay-v6-0-74f21d440819@nexthop.ai?part=6