Re: [PATCH] bpf, cpumap: Fix use after free of bpf_cpu_map_entry in cpu_map_enqueue

[Hou Tao]

On 7/27/2024 2:55 AM, Toke Høiland-Jørgensen wrote:
> Radoslaw Zielonek <radoslaw.zielonek@xxxxxxxxx> writes:
>
>> When cpu_map has been redirected, first the pointer to the
>> bpf_cpu_map_entry has been copied, then freed, and read from the copy.
>> To fix it, this commit introduced the refcount cpu_map_parent during
>> redirections to prevent use after free.
>>
>> syzbot reported:
>>
>> [   61.581464][T11670] ==================================================================
>> [   61.583323][T11670] BUG: KASAN: slab-use-after-free in cpu_map_enqueue+0xba/0x370
>> [   61.585419][T11670] Read of size 8 at addr ffff888122d75208 by task syzbot-repro/11670
>> [   61.587541][T11670]
>> [   61.588237][T11670] CPU: 1 PID: 11670 Comm: syzbot-repro Not tainted 6.9.0-rc6-00053-g0106679839f7 #27
>> [   61.590542][T11670] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.1 11/11/2019

SNIP
>> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
>> index a8e34416e960..0034a6d423b6 100644
>> --- a/kernel/bpf/cpumap.c
>> +++ b/kernel/bpf/cpumap.c
>> @@ -59,6 +59,9 @@ struct bpf_cpu_map_entry {
>>  	u32 cpu;    /* kthread CPU and map index */
>>  	int map_id; /* Back reference to map */
>>  
>> +	/* Used to end ownership transfer transaction */
>> +	struct bpf_map *parent_map;
>> +
>>  	/* XDP can run multiple RX-ring queues, need __percpu enqueue store */
>>  	struct xdp_bulk_queue __percpu *bulkq;
>>  
>> @@ -427,6 +430,7 @@ __cpu_map_entry_alloc(struct bpf_map *map, struct bpf_cpumap_val *value,
>>  	rcpu->cpu    = cpu;
>>  	rcpu->map_id = map->id;
>>  	rcpu->value.qsize  = value->qsize;
>> +	rcpu->parent_map = map;
>>  
>>  	if (fd > 0 && __cpu_map_load_bpf_program(rcpu, map, fd))
>>  		goto free_ptr_ring;
>> @@ -639,6 +643,14 @@ static int cpu_map_get_next_key(struct bpf_map *map, void *key, void *next_key)
>>  
>>  static long cpu_map_redirect(struct bpf_map *map, u64 index, u64 flags)
>>  {
>> +	/*
>> +	 * Redirection is a transfer of ownership of the bpf_cpu_map_entry
>> +	 * During the transfer the bpf_cpu_map_entry is still in the map,
>> +	 * so we need to prevent it from being freed.
>> +	 * The bpf_map_inc() increments the refcnt of the map, so the
>> +	 * bpf_cpu_map_entry will not be freed until the refcnt is decremented.
>> +	 */
>> +	bpf_map_inc(map);
> Adding refcnt increase/decrease in the fast path? Hard NAK.
>
> The map entry is protected by RCU, which should prevent this kind of UAF
> from happening. Looks like maybe there's a bug in the tun driver so this
> RCU protection is not working?

It will be possible if two different xdp programs set and use the value
of ri->tgt_vlaue separately as shown below:

(1) on CPU 0: xdp program A invokes bpf_redirect_map() (e.g., through
test_run) and sets ri->tgt_value as one entry in cpu map X
(2) release the xdp program A and the cpu map X is freed.
(3) on CPU 0: xdp program B doesn't invoke bpf_redirect_map(), but it
returns XDP_REDIRECT, so the old value of ri->tgt_value is used by
program B.

I think the problem is fixed after the merge of commit 401cb7dae813
("net: Reference bpf_redirect_info via task_struct on PREEMPT_RT"). 
Before the commit, bpf_redirect_info is a per-cpu variable, and
ri->tgt_value is not cleared when the running of xdp program completes,
so it is possible that one xdp program could use a stale tgt_values set
by other xdp program. After changing bpf_redirect_info into a per-thread
variable and clearing it after each run of xdp program, such sharing
will be impossible.

Zielonek, could you please help to check whether or not the problem is
reproducible in latest bpf-next tree ?

>
> -Toke
>
>
> .