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

[Peter Zijlstra <peterz@infradead.org>]

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.

(obviously, once we go do more complicated CFI schemes, all this needs
revisiting yet again).

I don't seem able to run this mod_race test, it keeps saying:

  tgl-build# ./test_progs -v -t mod_race
  bpf_testmod.ko is already unloaded.
  Loading bpf_testmod.ko...
  Successfully loaded bpf_testmod.ko.
  Summary: 0/0 PASSED, 0 SKIPPED, 0 FAILED
  Successfully unloaded bpf_testmod.ko.

Which I'm taking to mean I'm doing it wrong... so I can't immediately
verify, but your proposal looks sane so I'll fold it in.

Thanks!