Re: [PATCH bpf v2 1/2] bpf: Skip detached progs in trampoline images that are still in use

[Junseo Lim]

I think there's still a correctness gap here.

> > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
> > index c18e005a41dbe..9c166bdfbc6a6 100644
> > --- a/arch/arm64/net/bpf_jit_comp.c
> > +++ b/arch/arm64/net/bpf_jit_comp.c
> > @@ -2429,6 +2430,10 @@ static void invoke_bpf_prog(struct jit_ctx *ctx, struct bpf_tramp_node *node,
> >  	enter_prog = (u64)bpf_trampoline_enter(p);
> >  	exit_prog = (u64)bpf_trampoline_exit(p);
> >  
> > +	/* nop, patched to skip this prog when it is detached */
> > +	skip = ctx->ro_image + ctx->idx;
> > +	emit(A64_NOP, ctx);
> > +
> >  	if (node->cookie == 0) {
> >  		/* if cookie is zero, one instruction is enough to store it */
> >  		emit(A64_STR64I(A64_ZR, A64_SP, run_ctx_off + cookie_off), ctx);
> 
> [Severity: High]
> This is a pre-existing issue, but does this still leave a use-after-free
> window between the newly added skip NOP and the __bpf_prog_enter() call in
> invoke_bpf_prog()? 
> 
> If a task on a preemptible kernel executes this NOP but is involuntarily
> preempted before calling __bpf_prog_enter() (where rcu_read_lock or 
> rcu_read_lock_trace would be acquired), it hasn't blocked the RCU grace 
> periods yet. 
> 
> If another CPU detaches the program, patches the NOP, and drops the program 
> reference during this preemption, the program could be freed. When the
> preempted task resumes, could it load the now-freed program pointer and call
> __bpf_prog_enter(p) on freed memory?

I reproduced the scenario Sashiko pointed out in our environment:

     ==================================================================
     BUG: KASAN: vmalloc-out-of-bounds in __bpf_prog_enter_recur+0x3a5/0x3f0
     Read of size 8 at addr ffffc90000055040 by task candidate/110

     CPU: 1 UID: 0 PID: 110 Comm: candidate Not tainted 7.3.0-rc2-00014-g15071f2a1263-dirty #2 PREEMPT(full)
     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.17.0-2-2 04/01/2014
     Call Trace:
      <TASK>
      dump_stack_lvl+0xb0/0x110
      print_report+0x14b/0x4a4
      kasan_report+0x108/0x130
      ? __bpf_prog_enter_recur+0x3a5/0x3f0
      ? __bpf_prog_enter_recur+0x3a5/0x3f0
      __bpf_prog_enter_recur+0x3a5/0x3f0
      bpf_trampoline_6442509193+0x37/0xf1
      __x64_sys_futex+0x9/0x410
      do_syscall_64+0xb0/0x530
      ? srso_alias_return_thunk+0x5/0xfbef5
      entry_SYSCALL_64_after_hwframe+0x76/0x7e
     RIP: 0033:0x42a21d
     Code: d5 48 8d 3c 0a eb 91 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 c7 c1 e8 ff ff ff f7 d8 64 89 01 48
     RSP: 002b:00007f1befda9128 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
     RAX: ffffffffffffffda RBX: 00007f1befda9ce4 RCX: 000000000042a21d
     RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000000
     RBP: 00007f1befda92b0 R08: 0000000000000000 R09: 0000000000000000
     R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000021
     R13: 00007ffd400508a0 R14: 0000000000000010 R15: 00007ffd40050997
      </TASK>

     The buggy address belongs to a vmalloc virtual mapping
     Memory state around the buggy address:
      ffffc90000054f00: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
      ffffc90000054f80: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
     >ffffc90000055000: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
                                                ^
      ffffc90000055080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
      ffffc90000055100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8
     ==================================================================

I used SCHED_DEADLINE to increase the likelihood of preemption.
This seems consistent with the preemption window described above.

Best,
Junseo