Re: [PATCH v4 00/45] x86: Kernel IBT

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, Mar 16, 2022 at 03:05:38PM IST, Peter Zijlstra wrote:
> On Tue, Mar 15, 2022 at 10:00:43AM +0100, Peter Zijlstra wrote:
> > On Tue, Mar 15, 2022 at 02:14:02AM +0530, Kumar Kartikeya Dwivedi wrote:
> >
> > > [ Note: I have no experience with trampoline code or IBT so what follows might
> > > 	be incorrect. ]
> > >
> > > In case of fexit and fmod_ret, we call original function (but skip
> > > X86_PATCH_SIZE bytes), with ENDBR we must also skip those 4 bytes, but in some
> > > cases like bpf_fentry_test1, for which this test has fmod_ret prog, compiler
> > > (gcc 11) emits endbr64, but not for do_init_module, for which we do fexit.
> > >
> > > This means for do_init_module module, orig_call += X86_PATCH_SIZE +
> > > ENDBR_INSN_SIZE would skip more bytes than needed to emit call to original
> > > function, which explains why I was seeing crash in the middle of
> > > 'mov edx, 0x10' instruction.
> > >
> > > The diff below fixes the problem for me, and allows the test to pass.
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index b98e1c95bcc4..760c9a3c075f 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -2031,11 +2031,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > >
> > >         ip_off = stack_size;
> > >
> > > -       if (flags & BPF_TRAMP_F_SKIP_FRAME)
> > > +       if (flags & BPF_TRAMP_F_SKIP_FRAME) {
> > >                 /* skip patched call instruction and point orig_call to actual
> > >                  * body of the kernel function.
> > >                  */
> > > -               orig_call += X86_PATCH_SIZE + ENDBR_INSN_SIZE;
> > > +               if (is_endbr(*(u32 *)orig_call))
> > > +                       orig_call += ENDBR_INSN_SIZE;
> > > +               orig_call += X86_PATCH_SIZE;
> > > +       }
> > >
> > >         prog = image;
> >
> > Hmm, so I was under the impression that this was targeting the NOP from
> > emit_prologue(), and that has an unconditional ENDBR. If this is instead
> > targeting the 'start of random kernel function' then yes, what you
> > propose will work.
>
> Can you confirm that orig_call can be any kernel function? Because if
> so, I'm thinking it will still do the wrong thing for a notrace
> function, that will not have a __fentry__ site, so unconditionally
> skipping those 5 bytes will place you somewhere non-sensible.
>

It fails for notrace functions, e.g. considering fentry prog, when
bpf_trampoline_link_prog -> bpf_trampoline_update eventually calls
register_fentry -> bpf_arch_text_poke, old_addr is NULL, so nop_insn is copied
to old_insn, and then that memcmp(ip, old_insn, X86_PATCH_SIZE) should fail, so
it would return -EBUSY. If you consider modify_fentry case, then register_fentry
for earlier prog must have succeeded, so it must not be notrace function.

The orig_call adjustment is only done for fexit and fmod_ret (they set CALL_ORIG
and SKIP_FRAME flags), because in case of just fentry we just continue after
ret, instead of emitting call to original function in trampoline, for those too
the bpf_arch_text_poke should fail, for the same reason as above.

> This would not be a new issue; but perhaps it should be clarified and or
> fixed.

Based on my inspection it looks fine, others can correct me if I'm wrong.

--
Kartikeya