Re: [PATCH bpf v2 1/2] bpf: Fix crash due to incorrect copy_map_value

[Yonghong Song <yhs@fb.com>]

On 2/10/22 2:49 PM, Alexei Starovoitov wrote:
> On Thu, Feb 10, 2022 at 12:05 AM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>> On 2/9/22 11:52 AM, Kumar Kartikeya Dwivedi wrote:
>>> On Thu, Feb 10, 2022 at 12:36:08AM IST, Yonghong Song wrote:
>>>>
>>>>
>>>> On 2/8/22 11:03 PM, Kumar Kartikeya Dwivedi wrote:
>>>>> When both bpf_spin_lock and bpf_timer are present in a BPF map value,
>>>>> copy_map_value needs to skirt both objects when copying a value into and
>>>>> out of the map. However, the current code does not set both s_off and
>>>>> t_off in copy_map_value, which leads to a crash when e.g. bpf_spin_lock
>>>>> is placed in map value with bpf_timer, as bpf_map_update_elem call will
>>>>> be able to overwrite the other timer object.
>>>>>
>>>>> When the issue is not fixed, an overwriting can produce the following
>>>>> splat:
>>>>>
>>>>> [root@(none) bpf]# ./test_progs -t timer_crash
>>>>> [   15.930339] bpf_testmod: loading out-of-tree module taints kernel.
>>>>> [   16.037849] ==================================================================
>>>>> [   16.038458] BUG: KASAN: user-memory-access in __pv_queued_spin_lock_slowpath+0x32b/0x520
>>>>> [   16.038944] Write of size 8 at addr 0000000000043ec0 by task test_progs/325
>>>>> [   16.039399]
>>>>> [   16.039514] CPU: 0 PID: 325 Comm: test_progs Tainted: G           OE     5.16.0+ #278
>>>>> [   16.039983] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ArchLinux 1.15.0-1 04/01/2014
>>>>> [   16.040485] Call Trace:
>>>>> [   16.040645]  <TASK>
>>>>> [   16.040805]  dump_stack_lvl+0x59/0x73
>>>>> [   16.041069]  ? __pv_queued_spin_lock_slowpath+0x32b/0x520
>>>>> [   16.041427]  kasan_report.cold+0x116/0x11b
>>>>> [   16.041673]  ? __pv_queued_spin_lock_slowpath+0x32b/0x520
>>>>> [   16.042040]  __pv_queued_spin_lock_slowpath+0x32b/0x520
>>>>> [   16.042328]  ? memcpy+0x39/0x60
>>>>> [   16.042552]  ? pv_hash+0xd0/0xd0
>>>>> [   16.042785]  ? lockdep_hardirqs_off+0x95/0xd0
>>>>> [   16.043079]  __bpf_spin_lock_irqsave+0xdf/0xf0
>>>>> [   16.043366]  ? bpf_get_current_comm+0x50/0x50
>>>>> [   16.043608]  ? jhash+0x11a/0x270
>>>>> [   16.043848]  bpf_timer_cancel+0x34/0xe0
>>>>> [   16.044119]  bpf_prog_c4ea1c0f7449940d_sys_enter+0x7c/0x81
>>>>> [   16.044500]  bpf_trampoline_6442477838_0+0x36/0x1000
>>>>> [   16.044836]  __x64_sys_nanosleep+0x5/0x140
>>>>> [   16.045119]  do_syscall_64+0x59/0x80
>>>>> [   16.045377]  ? lock_is_held_type+0xe4/0x140
>>>>> [   16.045670]  ? irqentry_exit_to_user_mode+0xa/0x40
>>>>> [   16.046001]  ? mark_held_locks+0x24/0x90
>>>>> [   16.046287]  ? asm_exc_page_fault+0x1e/0x30
>>>>> [   16.046569]  ? asm_exc_page_fault+0x8/0x30
>>>>> [   16.046851]  ? lockdep_hardirqs_on+0x7e/0x100
>>>>> [   16.047137]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>>>>> [   16.047405] RIP: 0033:0x7f9e4831718d
>>>>> [   16.047602] Code: b4 0c 00 0f 05 eb a9 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d b3 6c 0c 00 f7 d8 64 89 01 48
>>>>> [   16.048764] RSP: 002b:00007fff488086b8 EFLAGS: 00000206 ORIG_RAX: 0000000000000023
>>>>> [   16.049275] RAX: ffffffffffffffda RBX: 00007f9e48683740 RCX: 00007f9e4831718d
>>>>> [   16.049747] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00007fff488086d0
>>>>> [   16.050225] RBP: 00007fff488086f0 R08: 00007fff488085d7 R09: 00007f9e4cb594a0
>>>>> [   16.050648] R10: 0000000000000000 R11: 0000000000000206 R12: 00007f9e484cde30
>>>>> [   16.051124] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>>>>> [   16.051608]  </TASK>
>>>>> [   16.051762] ==================================================================
>>>>>
>>>>> Fixes: 68134668c17f ("bpf: Add map side support for bpf timers.")
>>>>> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>>>>> ---
>>>>>     include/linux/bpf.h | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>>>>> index fa517ae604ad..31a83449808b 100644
>>>>> --- a/include/linux/bpf.h
>>>>> +++ b/include/linux/bpf.h
>>>>> @@ -224,7 +224,8 @@ static inline void copy_map_value(struct bpf_map *map, void *dst, void *src)
>>>>>      if (unlikely(map_value_has_spin_lock(map))) {
>>>>>              s_off = map->spin_lock_off;
>>>>>              s_sz = sizeof(struct bpf_spin_lock);
>>>>> -   } else if (unlikely(map_value_has_timer(map))) {
>>>>> +   }
>>>>> +   if (unlikely(map_value_has_timer(map))) {
>>>>>              t_off = map->timer_off;
>>>>>              t_sz = sizeof(struct bpf_timer);
>>>>>      }
>>>>
>>>> Thanks for the patch. I think we have a bigger problem here with the patch.
>>>> It actually exposed a few kernel bugs. If you run current selftests, esp.
>>>> ./test_progs -j which is what I tried, you will observe
>>>> various testing failures. The reason is due to we preserved the timer or
>>>> spin lock information incorrectly for a map value.
>>>>
>>>> For example, the selftest #179 (timer) will fail with this patch and
>>>> the following change can fix it.
>>>>
>>>
>>> I actually only saw the same failures (on bpf/master) as in CI, and it seems
>>> they are there even when I do a run without my patch (related to uprobes). The
>>> bpftool patch PR in GitHub also has the same error, so I'm guessing it is
>>> unrelated to this. I also didn't see any difference when running on bpf-next.
>>>
>>> As far as others are concerned, I didn't see the failure for timer test, or any
>>> other ones, for me all timer tests pass properly after applying it. It could be
>>> that my test VM is not triggering it, because it may depend on the runtime
>>> system/memory values, etc.
>>>
>>> Can you share what error you see? Does it crash or does it just fail?
>>
>> For test #179 (timer), most time I saw a hung. But I also see
>> the oops in bpf_timer_set_callback().
>>
>>>
>>>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>>>> index d29af9988f37..3336d76cc5a6 100644
>>>> --- a/kernel/bpf/hashtab.c
>>>> +++ b/kernel/bpf/hashtab.c
>>>> @@ -961,10 +961,11 @@ static struct htab_elem *alloc_htab_elem(struct
>>>> bpf_htab *htab, void *key,
>>>>                           l_new = ERR_PTR(-ENOMEM);
>>>>                           goto dec_count;
>>>>                   }
>>>> -               check_and_init_map_value(&htab->map,
>>>> -                                        l_new->key + round_up(key_size,
>>>> 8));
>>>>           }
>>>>
>>>> +       check_and_init_map_value(&htab->map,
>>>> +                                l_new->key + round_up(key_size, 8));
>>>> +
>>>
>>> Makes sense, but trying to understand why it would fail:
>>> So this is needed because the reused element from per-CPU region might have
>>> garbage in the bpf_spin_lock/bpf_timer fields? But I think atleast for timer
>>> case, we reset timer->timer to NULL in bpf_timer_cancel_and_free.
>>>
>>> Earlier copy_map_value further below in this code would also overwrite the timer
>>> part (which usually may be zero), but that would also not happen anymore.
>>
>> That is correct. The preallocated hash tables have a free list. Look
>> like when an element is put into a free list, its value is not reset.
> 
> I don't follow. How do you think it can happen?
> htab_delete/update are calling free_htab_elem()
> which calls check_and_free_timer().
> For pre-alloc htab_update calls check_and_free_timer() directly.
> There should be never a case when timer is active in the free list.

The issue is not a timer active in the free list. It is the timer value
is not reset to 0 in the free list.
For example,
  1. value->timer... is set properly (non zero)
  2. value is deleted through update or delete, value->timer
     is cancelled and freed, and the hash_elem is put into
     free list. But the hash_elem value->timer is not zero.
  3. one hash_elem is picked up from the free list,
     and proper value is copied to the value except value->timer
     and value->spinlock (if they exist). This happens with this patch.
  4. some later kernel functions may see value->timer is set and
     do something bad ...