Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <YifZhUVoHLT/76fE@hirez.programming.kicks-ass.net>
 <Yif8nO2xg6QnVQfD@hirez.programming.kicks-ass.net> <20220309190917.w3tq72alughslanq@ast-mbp.dhcp.thefacebook.com>
 <YinGZObp37b27LjK@hirez.programming.kicks-ass.net> <YioBZmicMj7aAlLf@hirez.programming.kicks-ass.net>
 <YionV0+v/cUBiOh0@hirez.programming.kicks-ass.net> <YisnG9lW6kp8lBp3@hirez.programming.kicks-ass.net>
 <CAADnVQJfffD9tH_cWThktCCwXeoRV1XLZq69rKK5vKy_y6BN8A@mail.gmail.com>
 <20220312154407.GF28057@worktop.programming.kicks-ass.net>
 <CAADnVQL7xrafAviUJg47LfvFSJpgZLwyP18Bm3S_KQwRyOpheQ@mail.gmail.com> <20220314204402.rpd5hqzzev4ugtdt@apollo>
In-Reply-To: <20220314204402.rpd5hqzzev4ugtdt@apollo>
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
Date:   Tue, 15 Mar 2022 11:26:10 -0700
Message-ID: <CAADnVQ+TMPpwEc_S7ayijzem-SOCQzuAeJAX=3mQXqgTPBW22A@mail.gmail.com>
Subject: Re: [PATCH v4 00/45] x86: Kernel IBT
To:     Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc:     Peter Zijlstra <peterz@infradead.org>, X86 ML <x86@kernel.org>,
        joao@overdrivepizza.com, hjl.tools@gmail.com,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Kees Cook <keescook@chromium.org>,
        Sami Tolvanen <samitolvanen@google.com>,
        Mark Rutland <mark.rutland@arm.com>, alyssa.milburn@intel.com,
        Miroslav Benes <mbenes@suse.cz>,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Mar 14, 2022 at 1:44 PM Kumar Kartikeya Dwivedi
<memxor@gmail.com> 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.