Re: [PATCH bpf-next v2 3/3] bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker

[email protected] writes:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] A race condition between dropping the arena spinlock and flushing the TLB allows concurrent reallocation and stale TLB access, leading to an information leak.
> --
>
> --- Patch [3]: [PATCH bpf-next v2 3/3] bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker ---
> commit d52cc998404e8cd2b8a37ee307bab5c587ce8336
> Author: Yuan Chen <[email protected]>
>
> bpf, arena: check range_tree_set return in arena_free_pages and arena_free_worker
>
> arena_free_pages() and arena_free_worker() now handle range_tree_set()
> errors. arena_free_pages() aborts the free on error, and
> arena_free_worker() moves range_tree_set() before PTE clearing so that a
> failed tree update leaves the PTEs intact instead of freeing pages that
> the arena free tree does not track.
>
>> diff --git a/kernel/bpf/arena.c b/kernel/bpf/arena.c
>> index 7b6847200b431..4597d9becb97a 100644
>> --- a/kernel/bpf/arena.c
>> +++ b/kernel/bpf/arena.c
>> @@ -881,7 +881,12 @@ static void arena_free_pages(struct bpf_arena *arena, long uaddr, long page_cnt,
>>  	if (ret)
>>  		goto defer;
>>  
>> -	range_tree_set(&arena->rt, pgoff, page_cnt);
>> +	ret = range_tree_set(&arena->rt, pgoff, page_cnt);
>> +	if (ret) {
>> +		raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>> +		bpf_map_memcg_exit(old_memcg, new_memcg);
>> +		return;
>> +	}
>
> [Severity: High]
> This is a pre-existing issue, but does marking the range as free here,
> before dropping the spinlock and flushing the TLB, create a race condition?
>
> If CPU 1 calls arena_free_pages(), marks the pages as free via
> range_tree_set(), clears the PTEs, and drops the spinlock:
>
>         ret = range_tree_set(&arena->rt, pgoff, page_cnt);
>         ...
>         raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>
> Before CPU 1 executes flush_tlb_kernel_range(), could CPU 2 allocate the
> same virtual address via bpf_arena_alloc_pages() and populate new PTEs? 
>
> If CPU 2 (or another CPU) previously accessed this virtual address, its
> TLB might still point to the old physical page. Writes to this address
> would then incorrectly modify the old physical page.
>
> CPU 1 would then flush the TLB and free the old physical page to the buddy
> allocator, potentially leaking sensitive data to other processes.
>
> [ ... ]
>
>> @@ -1004,11 +1009,18 @@ static void arena_free_worker(struct work_struct *work)
>>  		kaddr = arena_vm_start + s->uaddr;
>>  		pgoff = compute_pgoff(arena, s->uaddr);
>>  
>> +		/*
>> +		 * Mark the range as free before clearing PTEs so that
>> +		 * if the range tree update fails we leave the PTEs
>> +		 * intact, avoiding an unrecoverable state where pages
>> +		 * are freed but the arena free tree does not track them.
>> +		 */
>> +		if (range_tree_set(&arena->rt, pgoff, page_cnt))
>> +			continue;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but does the same race
> condition exist here in the asynchronous worker?
>
> The worker marks the range as free via range_tree_set() and drops the
> spinlock:
>
>         if (range_tree_set(&arena->rt, pgoff, page_cnt))
>                 continue;
>         ...
>         raw_res_spin_unlock_irqrestore(&arena->spinlock, flags);
>
> Before iterating the list again to call flush_tlb_kernel_range(), this
> seems to expose the old physical pages to the same concurrent reallocation
> race window as arena_free_pages().
>

Both these issues are real and I am working on a fix for these. I
already saw them while fuzzing the BPF JITs through litmus tests.

Will post the patch soon.

Thanks,
Puranjay