Re: [PATCH bpf-next v3] bpf: fix NULL pointer dereference in bpf_get_local_storage() helper

[Yonghong Song <yhs@fb.com>]

On 3/22/21 1:47 PM, Jiri Olsa wrote:
> On Sat, Mar 20, 2021 at 10:02:01AM -0700, Yonghong Song wrote:
>> Jiri Olsa reported a bug ([1]) in kernel where cgroup local
>> storage pointer may be NULL in bpf_get_local_storage() helper.
>> There are two issues uncovered by this bug:
>>    (1). kprobe or tracepoint prog incorrectly sets cgroup local storage
>>         before prog run,
>>    (2). due to change from preempt_disable to migrate_disable,
>>         preemption is possible and percpu storage might be overwritten
>>         by other tasks.
>>
>> This issue (1) is fixed in [2]. This patch tried to address issue (2).
>> The following shows how things can go wrong:
>>    task 1:   bpf_cgroup_storage_set() for percpu local storage
>>           preemption happens
>>    task 2:   bpf_cgroup_storage_set() for percpu local storage
>>           preemption happens
>>    task 1:   run bpf program
>>
>> task 1 will effectively use the percpu local storage setting by task 2
>> which will be either NULL or incorrect ones.
>>
>> Instead of just one common local storage per cpu, this patch fixed
>> the issue by permitting 8 local storages per cpu and each local
>> storage is identified by a task_struct pointer. This way, we
>> allow at most 8 nested preemption between bpf_cgroup_storage_set()
>> and bpf_cgroup_storage_unset(). The percpu local storage slot
>> is released (calling bpf_cgroup_storage_unset()) by the same task
>> after bpf program finished running.
>> bpf_test_run() is also fixed to use the new bpf_cgroup_storage_set()
>> interface.
>>
>> The patch is tested on top of [2] with reproducer in [1].
>> Without this patch, kernel will emit error in 2-3 minutes.
>> With this patch, after one hour, still no error.
>>
>>   [1] https://lore.kernel.org/bpf/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T
>>   [2] https://lore.kernel.org/bpf/CAKH8qBuXCfUz=w8L+Fj74OaUpbosO29niYwTki7e3Ag044_aww@mail.gmail.com/T
> 
> [1] and [2] are same link, you mean this one, right?
>     05a68ce5fa51 bpf: Don't do bpf_cgroup_storage_set() for kuprobe/tp programs

Thanks for spotting this! Will fix it and submit v4.

> 
> I have troubles to apply this on bpf-next probably because
> of dependencies, I'll wait for bpf-next is in sync with bpf
> fixes.. or would you have a branch pushed out with this?

I do not have a public branch for this. You can do:
   - with latest bpf-next
   - cherry-pick 05a68ce5fa51 bpf: Don't do bpf_cgroup_storage_set() for 
kuprobe/tp programs
   - apply this patch
Then you should be good to go to do a testing. Thanks!

> 
> thanks for the fix,
> jirka
> 
>>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Roman Gushchin <guro@fb.com>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   include/linux/bpf-cgroup.h | 57 ++++++++++++++++++++++++++++++++------
>>   include/linux/bpf.h        | 22 ++++++++++++---
>>   kernel/bpf/helpers.c       | 15 +++++++---
>>   kernel/bpf/local_storage.c |  5 ++--
>>   net/bpf/test_run.c         |  6 +++-
>>   5 files changed, 86 insertions(+), 19 deletions(-)
>>
[...]