Re: [PATCH bpf] bpf: Fix invalid mem access when update_effective_progs fails in __cgroup_bpf_detach

[Pu Lehui <pulehui@huawei.com>]

On 2025/11/6 10:14, Pu Lehui wrote:
> 
> 
> On 2025/11/6 7:33, Eduard Zingerman wrote:
>> On Wed, 2025-11-05 at 10:03 +0000, Pu Lehui wrote:
>>> From: Pu Lehui <pulehui@huawei.com>
>>>
>>> Syzkaller triggers an invalid memory access issue following fault
>>> injection in update_effective_progs. The issue can be described as
>>> follows:
>>>
>>> __cgroup_bpf_detach
>>>    update_effective_progs
>>>      compute_effective_progs
>>>        bpf_prog_array_alloc <-- fault inject
>>>    purge_effective_progs
>>>      /* change to dummy_bpf_prog */
>>>      array->items[index] = &dummy_bpf_prog.prog
>>>
>>> ---softirq start---
>>> __do_softirq
>>>    ...
>>>      __cgroup_bpf_run_filter_skb
>>>        __bpf_prog_run_save_cb
>>>          bpf_prog_run
>>>            stats = this_cpu_ptr(prog->stats)
>>>            /* invalid memory access */
>>>            flags = u64_stats_update_begin_irqsave(&stats->syncp)
>>> ---softirq end---
>>>
>>>    static_branch_dec(&cgroup_bpf_enabled_key[atype])
>>>
>>> The reason is that fault injection caused update_effective_progs to fail
>>> and then changed the original prog into dummy_bpf_prog.prog in
>>> purge_effective_progs. Then a softirq came, and accessing the members of
>>> dummy_bpf_prog.prog in the softirq triggers invalid mem access.
>>>
>>> To fix it, we can skip executing the prog when it's dummy_bpf_prog.prog.
>>>
>>> Fixes: 4c46091ee985 ("bpf: Fix KASAN use-after-free Read in 
>>> compute_effective_progs")
>>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
>>
>> Is there a link for syzkaller report?
> 
> 
> Hi Eduard,
> 
> This is a local syzkaller test, and I have attached the report at the 
> end of the email.
> 
>>
>> [...]
>>
>>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
>>> index 248f517d66d0..baad33b34cef 100644
>>> --- a/kernel/bpf/cgroup.c
>>> +++ b/kernel/bpf/cgroup.c
>>> @@ -77,7 +77,9 @@ bpf_prog_run_array_cg(const struct cgroup_bpf *cgrp,
>>>       item = &array->items[0];
>>>       old_run_ctx = bpf_set_run_ctx(&run_ctx.run_ctx);
>>>       while ((prog = READ_ONCE(item->prog))) {
>>> -        run_ctx.prog_item = item;
>>> +        run_ctx.prog_item = item++;
>>> +        if (prog == &dummy_bpf_prog.prog)
>>> +            continue;
>>
>> Will the following fix the issue?
>>
>>      diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
>>      index d595fe512498..c7c9c78f171a 100644
>>      --- a/kernel/bpf/core.c
>>      +++ b/kernel/bpf/core.c
>>      @@ -2536,11 +2536,14 @@ static unsigned int __bpf_prog_ret1(const 
>> void *ctx,
>>              return 1;
>>       }
>>
>>      +DEFINE_PER_CPU(struct bpf_prog_stats, __dummy_stats);
>>      +
>>       static struct bpf_prog_dummy {
>>              struct bpf_prog prog;
>>       } dummy_bpf_prog = {
>>              .prog = {
>>                      .bpf_func = __bpf_prog_ret1,
>>      +               .stats = &__dummy_stats,
>>              },
>>       };
>>
>> Or that's too much memory wasted?
> 
> In 160 cores system, it will waste 5K bytes for this dummy.
> 
> And also, this solution will not suit for 5.10.0 or lower LTS version, 
> as the bpf_prog_stats is embedded in struct bpf_prog_aux, and bpf->aux 
> is empty at this time, which will trigger a null pointer access.

Hi Eduard,

I've reviewed the kernel usage of static per-CPU variables and believe 
that 32 bytes per core is not a significant overhead. Moreover, similar 
approaches can be applied to older versions. I've submitted the v2 based 
on your suggestions.

https://lore.kernel.org/bpf/20251110071714.4069712-1-pulehui@huaweicloud.com/

Thanks.