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

["Abdurrahman Hussain" <abdurrahman@nexthop.ai>]

On Mon Aug 31, 2026 at 6:45 PM PDT, sashiko-bot wrote:
> 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.
>

Confirmed. of_overlay_remove_all() is the one caller that passes
&ovcs->id, so the "*ovcs_id = 0" in of_overlay_remove() zeroes
ovcs->id itself before free_overlay_changeset() runs. The
"if (ovcs->id > 0)" guard (previously "if (ovcs->id)") then skips
idr_remove() and list_del(), and the ovcs is kfree()d while still
linked in ovcs_list and registered in ovcs_idr. The next ovcs_list or
ovcs_idr access after that walks freed memory.

Pre-existing — the ordering dates to 24789c5ce5a3 ("of: overlay:
detect cases where device tree may become corrupt") — and latent, since
of_overlay_remove_all() currently has no in-tree callers. Still worth
fixing for module users. The minimal fix is to stop aliasing the
changeset's own field:

      list_for_each_entry_safe_reverse(ovcs, ovcs_n, &ovcs_list, ovcs_list) {
-             ret = of_overlay_remove(&ovcs->id);
+             int id = ovcs->id;
+
+             ret = of_overlay_remove(&id);

Rob, want this as one more patch in this series (it sits right next to
patch 5), or as a separate fix?

>
> [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;
>>  	}

Known — you raised the same walk on v6, and it was deliberately left
out of this series' scope. It also can't be fixed locally: wrapping
the loop in of_mutex deadlocks against of_overlay_remove() taking
of_mutex itself, so it needs a __of_overlay_remove() split. That is
follow-up material, and equally unreachable in-tree today for the same
no-callers reason.

Thanks,
Abdurrahman