Re: [PATCH bpf] x86/bpf: handle bpf-program-triggered exceptions properly

[Yonghong Song <yhs@fb.com>]

On 1/27/21 1:47 PM, Andy Lutomirski wrote:
> On Mon, Jan 25, 2021 at 4:12 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> When reviewing patch ([1]), which adds a script to run bpf selftest
>> through qemu at /sbin/init stage, I found the following kernel bug
>> warning:
>>
>> [  112.118892] BUG: sleeping function called from invalid context at arch/x86/mm/fault.c:1351
>> [  112.119805] in_atomic(): 0, irqs_disabled(): 0, non_block: 0, pid: 354, name: new_name
>> [  112.120512] 3 locks held by new_name/354:
>> [  112.120868]  #0: ffff88800476e0a0 (&p->lock){+.+.}-{3:3}, at: bpf_seq_read+0x3a/0x3d0
>> [  112.121573]  #1: ffffffff82d69800 (rcu_read_lock){....}-{1:2}, at: bpf_iter_run_prog+0x5/0x160
>> [  112.122348]  #2: ffff8880061c2088 (&mm->mmap_lock#2){++++}-{3:3}, at: exc_page_fault+0x1a1/0x640
>> [  112.123128] Preemption disabled at:
>> [  112.123130] [<ffffffff8108f913>] migrate_disable+0x33/0x80
>> [  112.123942] CPU: 0 PID: 354 Comm: new_name Tainted: G           O      5.11.0-rc4-00524-g6e66fbb
>> 10597-dirty #1249
>> [  112.124822] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-1.el7.centos 04/01
>> /2014
>> [  112.125614] Call Trace:
>> [  112.125835]  dump_stack+0x77/0x97
>> [  112.126137]  ___might_sleep.cold.119+0xf2/0x106
>> [  112.126537]  exc_page_fault+0x1c1/0x640
>> [  112.126888]  asm_exc_page_fault+0x1e/0x30
>> [  112.127241] RIP: 0010:bpf_prog_0a182df2d34af188_dump_bpf_prog+0xf5/0xb3c
>> [  112.127825] Code: 00 00 8b 7d f4 41 8b 76 44 48 39 f7 73 06 48 01 fb 49 89 df 4c 89 7d d8 49 8b
>> bd 20 01 00 00 48 89 7d e0 49 8b bd e0 00 00 00 <48> 8b 7f 20 48 01 d7 48 89 7d e8 48 89 e9 48 83 c
>> 1 d0 48 8b 7d c8
>> [  112.129433] RSP: 0018:ffffc9000035fdc8 EFLAGS: 00010282
>> [  112.129895] RAX: 0000000000000000 RBX: ffff888005a49458 RCX: 0000000000000024
>> [  112.130509] RDX: 00000000000002f0 RSI: 0000000000000509 RDI: 0000000000000000
>> [  112.131126] RBP: ffffc9000035fe20 R08: 0000000000000001 R09: 0000000000000000
>> [  112.131737] R10: 0000000000000002 R11: 0000000000000000 R12: 0000000000000400
>> [  112.132355] R13: ffff888006085800 R14: ffff888004718540 R15: ffff888005a49458
>> [  112.132990]  ? bpf_prog_0a182df2d34af188_dump_bpf_prog+0xc8/0xb3c
>> [  112.133526]  bpf_iter_run_prog+0x75/0x160
>> [  112.133880]  __bpf_prog_seq_show+0x39/0x40
>> [  112.134258]  bpf_seq_read+0xf6/0x3d0
>> [  112.134582]  vfs_read+0xa3/0x1b0
>> [  112.134873]  ksys_read+0x4f/0xc0
>> [  112.135166]  do_syscall_64+0x2d/0x40
>> [  112.135482]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> To reproduce the issue, with patch [1] and use the following script:
>>    tools/testing/selftests/bpf/run_in_vm.sh -- cat /sys/fs/bpf/progs.debug
>>
>> The reason of the above kernel warning is due to bpf program
>> tries to dereference an address of 0 and which is not caught
>> by bpf exception handling logic.
>>
>> ...
>> SEC("iter/bpf_prog")
>> int dump_bpf_prog(struct bpf_iter__bpf_prog *ctx)
>> {
>>          struct bpf_prog *prog = ctx->prog;
>>          struct bpf_prog_aux *aux;
>>          ...
>>          if (!prog)
>>                  return 0;
>>          aux = prog->aux;
>>          ...
>>          ... aux->dst_prog->aux->name ...
>>          return 0;
>> }
>>
>> If the aux->dst_prog is NULL pointer, a fault will happen when trying
>> to access aux->dst_prog->aux.
>>
> 
> Which would be a bug in the bpf verifier, no?  This is a bpf program
> that apparently passed verification, and it somehow dereferenced a
> NULL pointer.
> 
> Let's enumerate some cases.
> 
> 1. x86-like architecture, SMAP enabled.  The CPU determines that this
> is bogus, and bpf gets lucky, because the x86 code runs the exception
> handler instead of summarily OOPSing.  IMO it would be entirely
> reasonable to OOPS.
> 
> 2 x86-like architecture, SMAP disabled.  This looks like a valid user
> access, and for all bpf knows, 0 might be an actual mapped address,
> and it might have userfaultfd attached, etc.  And it's called from a
> non-sleepable context.  The warning is entirely correct.
> 
> 3. s390x-like architecture.  This is a dereference of a bad kernel
> pointer.  OOPS, unless you get lucky.
> 
> 
> This patch is totally bogus.  You can't work around this by some magic
> exception handling.  Unless I'm missing something big, this is a bpf
> bug.  The following is not a valid kernel code pattern:
> 
> label:
>    dereference might-be-NULL kernel pointer
> _EXHANDLER_...(label, ...); /* try to recover */
> 
> This appears to have been introduced by:
> 
> commit 3dec541b2e632d630fe7142ed44f0b3702ef1f8c
> Author: Alexei Starovoitov <ast@kernel.org>
> Date:   Tue Oct 15 20:25:03 2019 -0700
> 
>      bpf: Add support for BTF pointers to x86 JIT
> 
> Alexei, this looks like a very long-winded and ultimately incorrect
> way to remove the branch from:
> 
> if (ptr)
>    val = *ptr;
> 
> Wouldn't it be better to either just emit the branch directly or to
> make sure that the pointer is valid in the first place?

Let me explain the motivation for this patch.

Previously, for any kernel data structure access,
people has to use bpf_probe_read() or bpf_probe_read_kernel()
helper even most of these accesses are okay and will not
fault. For example, for
    int t = a->b->c->d
three bpf_probe_read() will be needed, e.g.,
    bpf_probe_read_kernel(&t1, sizeof(t1), &a->b);
    bpf_probe_read_kernel(&t2, sizeof(t2), &t1->c);
    bpf_probe_read_kernel(&t, sizeof(t), &t2->d);

if there is a fault, bpf_probe_read_kernel() helper will
suppress the exception and clears the dest buffer and
return.

The above usage of bpf_probe_read_kernel()
is complicated and not C like and bpf developers
does not like it.

bcc (https://github.com/iovisor/bcc/) permits
users to write "a->b->c->d" styles and then through
clang rewriter to convert it to a series of
bpf_probe_read_kernel()'s. But most users are
directly using clang to compile their programs so
they have to write bpf_probe_read_kernel()'s
by themselves.

The motivation here is to improve user experience so
user can just write
    int t = a->b->c->d
some kernel will automatically take care of exceptions
and maintain bpf_probe_read_kernel() semantics.
So what the above patch essentially did is to check if the "regs->ip"
is one of ips which try to a "bpf_probe_read_kernel()" (actually
a direct access), it will fix up exception (clear the dest register)
and returns.

For a->b->c->d, some users may add "if (ptr) check"
for some of them and if that is the
case, compiler/verifier will honor that. Some users
may not add if they are certain from code that in most
or all cases, pointer will not be null. From verifier
perspective, it will be hard to decide whether
a->b, a->b->c is null or not, adding null checks
to every kernel de-references might be excessive. Also,
not 100% sure whether with null check, the pointer
dereference will absolutely not produce fault or not.
If there is no such guarantee then bpf_probe_read_kernel()
will be still needed.

I see page fault handler specially processed page fault for
kprobe and vsyscall. maybe page faults generated by special
bpf insns (simulating bpf_probe_read_kernel()) can also
be specially processed here.

> 
> --Andy
>