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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `of_overlay_remove_all()`, passing `&ovcs->id` to `of_overlay_remove()` causes a deterministic use-after-free and leaves freed memory in the global `ovcs_list` and `ovcs_idr`.
- [High] Lockless iteration of `ovcs_list` in `of_overlay_remove_all()` allows concurrent modifications, leading to a Use-After-Free.
--

commit 5e44e19c9c02429165243b3c23378b30ba40fdd3
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.

> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 08d5351746be6..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) {

[Severity: High]
This isn't a bug introduced by this patch, but does this check cause a 
use-after-free and list corruption when called from of_overlay_remove_all()?

In of_overlay_remove_all(), we iterate over the ovcs_list and pass the 
address of the internal id field:

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

Inside of_overlay_remove(), it zeros out this ID directly through the pointer 
before freeing the changeset:

drivers/of/overlay.c:of_overlay_remove() {
...
    *ovcs_id = 0;
...
    free_overlay_changeset(ovcs);
...
}

When free_overlay_changeset() executes, ovcs->id is already 0, so the 
if (ovcs->id > 0) condition will evaluate to false. This skips both 
idr_remove() and list_del(&ovcs->ovcs_list) before the memory is freed, 
leaving stale pointers in the active global list and IDR.


[Severity: High]
This is also a pre-existing issue, but could the lockless iteration in 
of_overlay_remove_all() lead to a use-after-free?

The iteration of ovcs_list in of_overlay_remove_all() allows concurrent 
modifications:

drivers/of/overlay.c:of_overlay_remove_all() {
    list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
...
}

This iterates the globally shared ovcs_list without holding of_mutex. If 
another thread concurrently calls of_overlay_remove() on a different overlay, 
it will acquire of_mutex and remove or free that overlay.

If the freed overlay was concurrently cached as ovcs_n (the 'next' element in 
the safe iteration), the loop in of_overlay_remove_all() will dereference 
the freed memory when advancing to the next iteration.

>  		idr_remove(&ovcs_idr, ovcs->id);
>  		list_del(&ovcs->ovcs_list);
>  		ovcs->id = 0;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-nh-of-alias-overlay-v7-0-02754604805a@nexthop.ai?part=5