Re: [PATCH bpf v3] selftests/bpf: Fix "missing ENDBR" BUG for destructor kfunc

[Chen, Hu1]

On 12/1/2022 7:50 PM, Jiri Olsa wrote:
>  On Thu, Dec 01, 2022 at 04:07:56PM +0800, Chen, Hu1 wrote:
> >  On 12/1/2022 12:52 AM, Yonghong Song wrote:
> > >   
> > >   
> > >   On 11/30/22 2:11 AM, Chen Hu wrote:
> > > > With CONFIG_X86_KERNEL_IBT enabled, the test_verifier triggers the
> > > > following BUG:
> > > >
> > > >    traps: Missing ENDBR: bpf_kfunc_call_test_release+0x0/0x30
> > > >    ------------[ cut here ]------------
> > > >    kernel BUG at arch/x86/kernel/traps.c:254!
> > > >    invalid opcode: 0000 [#1] PREEMPT SMP
> > > >    <TASK>
> > > >     asm_exc_control_protection+0x26/0x50
> > > >    RIP: 0010:bpf_kfunc_call_test_release+0x0/0x30
> > > >    Code: 00 48 c7 c7 18 f2 e1 b4 e8 0d ca 8c ff 48 c7 c0 00 f2 e1 b4 c3
> > > >     0f 1f 44 00 00 66 0f 1f 00 0f 1f 44 00 00 0f 0b 31 c0 c3 66 90
> > > >         <66> 0f 1f 00 0f 1f 44 00 00 48 85 ff 74 13 4c 8d 47 18 b8 ff ff ff
> > > >     bpf_map_free_kptrs+0x2e/0x70
> > > >     array_map_free+0x57/0x140
> > > >     process_one_work+0x194/0x3a0
> > > >     worker_thread+0x54/0x3a0
> > > >     ? rescuer_thread+0x390/0x390
> > > >     kthread+0xe9/0x110
> > > >     ? kthread_complete_and_exit+0x20/0x20
> > > >
> > > > It turns out that ENDBR in bpf_kfunc_call_test_release() is converted to
> > > > NOP by apply_ibt_endbr().
> > > >
> > > > The only text references to this function from kernel side are:
> > > >
> > > >    $ grep -r bpf_kfunc_call_test_release
> > > >    net/bpf/test_run.c:noinline void bpf_kfunc_call_test_release(...)
> > > >    net/bpf/test_run.c:BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, ...)
> > > >    net/bpf/test_run.c:BTF_ID(func, bpf_kfunc_call_test_release)
>   
>   Alexei mentioned we could now move these ^^^ into kernel module:
>     https://lore.kernel.org/bpf/CAADnVQJ4xaAacOUpzMG+bm2WK5u=1YLo5kLUL+RP3JZGW3Sfww@xxxxxxxxxxxxxx/
>   
>   would it help? not sure objtool scans modules as well
>   
>   but we'd still need fix for bpf_obj_new_impl/bpf_obj_drop_impl below
>   
>   jirka
>
Kernel module may not help on this issue.

If the kernel thinks some of the ENDBR instrucitons in module are
unneeded, it may remove them when load module. Let's take
bpf_testmod_test_mod_kfunc in bpf_testmod.ko as example.

- Disassembly in bpf_testmod.ko:
$ objdump -D bpf_testmod.ko | grep bpf_testmod_test_mod_kfunc -A1
00000000000005e0 <bpf_testmod_test_mod_kfunc>:
 5e0:   f3 0f 1e fa             endbr64
...

- Diassembly after kernel load bpf_testmod.ko
(gdb) disas bpf_testmod_test_mod_kfunc
Dump of assembler code for function bpf_testmod_test_mod_kfunc:
   0xffffffffc02015e0 <+0>:     nopw   (%rax)

Below backtrace shows how ENDBR is converted to nopw when load_module:
(gdb) bt
#0  apply_ibt_endbr (start=0xffffffffc0202316, end=  ...
#1  0xffffffff81096f19 in module_finalize (hdr=0xff  ...
#2  0xffffffff8118375e in post_relocation (info=0xf  ...
#3  load_module (info=info@entry=0xffa0000007843e08, ...

Thanks
Chen Hu
> > >   
> > >   We have some other function like this. For example, some newly added
> > >   functions like bpf_obj_new_impl(), bpf_obj_drop_impl(), do they have
> > >   the same missing endbr problem? If this is the case, we need a
> > >   general solution.
> > > 
> > 
> >   bpf_obj_new_impl(), bpf_obj_drop_impl() also miss the ENDBR. Below is
> >   the disassembly on bpf-next kernel:
> > 
> >   (gdb) disas bpf_obj_drop_impl
> >   Dump of assembler code for function bpf_obj_drop_impl:
> >      0xffffffff81288e40 <+0>:     nopw   (%rax)
> >      0xffffffff81288e44 <+4>:     nopl   0x0(%rax,%rax,1)
> >      0xffffffff81288e49 <+9>:     push   %rbp
> >      ...
> > 
> >   (gdb) disas bpf_obj_new_impl
> >   Dump of assembler code for function bpf_obj_new_impl:
> >      0xffffffff81288cd0 <+0>:     nopw   (%rax)
> >      0xffffffff81288cd4 <+4>:     nopl   0x0(%rax,%rax,1)
> >      0xffffffff81288cd9 <+9>:     push   %rbp
> >      ...
> > 
> >   The first insn in the bpf_obj_new_impl has been converted from ENDBR to
> >   nopw by objtool. If the function is indirectly called on IBT enabled CPU
> >   (Tigerlake for example), #CP raise.
> > 
> >   Looks like the possible fix in this patch is general?
> >   If we don't want to seal a funciton, we use macro IBT_NOSEAL to claim.
> >   IBT_NOSEAL just creates throwaway dummy compile-time references to the
> >   functions. The section is already thrown away when kernel run. See
> >   commit e27e5bea956c by Josh Poimboeuf.
> > 
> > > >
> > > > but it may be called from bpf program as kfunc. (no other caller from
> > > > kernel)
> > > >
> > > > This fix creates dummy references to destructor kfuncs so ENDBR stay
> > > > there.
> > > >
> > > > Also modify macro XXX_NOSEAL slightly:
> > > > - ASM_IBT_NOSEAL now stands for pure asm
> > > > - IBT_NOSEAL can be used directly in C
> > > >
> > > > Signed-off-by: Chen Hu <hu1.chen@xxxxxxxxx>
> > > > Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> > > > ---
> > > > v3:
> > > > - Macro go to IBT related header as suggested by Jiri Olsa
> > > > - Describe reference to the func clearly in commit message as suggested
> > > >    by Peter Zijlstra and Jiri Olsa
> > > >   v2: https://lore.kernel.org/all/20221122073244.21279-1-hu1.chen@xxxxxxxxx/
> > > >
> > > > v1: https://lore.kernel.org/all/20221121085113.611504-1-hu1.chen@xxxxxxxxx/
> > > >
> > > >   arch/x86/include/asm/ibt.h | 6 +++++-
> > > >   arch/x86/kvm/emulate.c     | 2 +-
> > > >   net/bpf/test_run.c         | 5 +++++
> > > >   3 files changed, 11 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/x86/include/asm/ibt.h b/arch/x86/include/asm/ibt.h
> > > > index 9b08082a5d9f..be86dc31661c 100644
> > > > --- a/arch/x86/include/asm/ibt.h
> > > > +++ b/arch/x86/include/asm/ibt.h
> > > > @@ -36,11 +36,14 @@
> > > >    * the function as needing to be "sealed" (i.e. ENDBR converted to NOP by
> > > >    * apply_ibt_endbr()).
> > > >    */
> > > > -#define IBT_NOSEAL(fname)                \
> > > > +#define ASM_IBT_NOSEAL(fname)                \
> > > >       ".pushsection .discard.ibt_endbr_noseal\n\t"    \
> > > >       _ASM_PTR fname "\n\t"                \
> > > >       ".popsection\n\t"
> > > >   +#define IBT_NOSEAL(name)                \
> > > > +    asm(ASM_IBT_NOSEAL(#name))
> > > > +
> > > >   static inline __attribute_const__ u32 gen_endbr(void)
> > > >   {
> > > >       u32 endbr;
> > > > @@ -94,6 +97,7 @@ extern __noendbr void ibt_restore(u64 save);
> > > >   #ifndef __ASSEMBLY__
> > > >     #define ASM_ENDBR
> > > > +#define ASM_IBT_NOSEAL(name)
> > > >   #define IBT_NOSEAL(name)
> > > >     #define __noendbr
> > > > diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> > > > index 4a43261d25a2..d870c8bb5831 100644
> > > > --- a/arch/x86/kvm/emulate.c
> > > > +++ b/arch/x86/kvm/emulate.c
> > > > @@ -327,7 +327,7 @@ static int fastop(struct x86_emulate_ctxt *ctxt, fastop_t fop);
> > > >       ".type " name ", @function \n\t" \
> > > >       name ":\n\t" \
> > > >       ASM_ENDBR \
> > > > -    IBT_NOSEAL(name)
> > > > +    ASM_IBT_NOSEAL(name)
> > > >     #define FOP_FUNC(name) \
> > > >       __FOP_FUNC(#name)
> > > > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > > > index fcb3e6c5e03c..9e9c8e8d50d7 100644
> > > > --- a/net/bpf/test_run.c
> > > > +++ b/net/bpf/test_run.c
> > > > @@ -601,6 +601,11 @@ noinline void bpf_kfunc_call_memb_release(struct prog_test_member *p)
> > > >   {
> > > >   }
> > > >   +#ifdef CONFIG_X86_KERNEL_IBT
> > > > +IBT_NOSEAL(bpf_kfunc_call_test_release);
> > > > +IBT_NOSEAL(bpf_kfunc_call_memb_release);
> > > > +#endif
> > > > +
> > > >   noinline void bpf_kfunc_call_memb1_release(struct prog_test_member1 *p)
> > > >   {
> > > >       WARN_ON_ONCE(1);