On Mon, Mar 14, 2022 at 1:44 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> The crash does not seem to be resurfacing the bug, AFAICT.
>
> [ 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;
> + }
>
Thanks Kumar!
The bpf trampoline can attach to both indirect and non-indirect
functions. My understanding is that only indirect targets will have
endbr first insn. So the fix totally makes sense.
