Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230401234144.3719742-1-xukuohai@huaweicloud.com>
In-Reply-To: <20230401234144.3719742-1-xukuohai@huaweicloud.com>
From:   Florent Revest <revest@chromium.org>
Date:   Mon, 3 Apr 2023 13:16:44 +0200
Message-ID: <CABRcYmJ7Oi6fpU76aNFpEwa94sZ+O7q6M1k-LMB0f0iXbFXGZQ@mail.gmail.com>
Subject: Re: [PATCH bpf-next] bpf, arm64: Fixed a BTI error on returning to
 patched function
To:     Xu Kuohai <xukuohai@huaweicloud.com>
Cc:     bpf@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sat, Apr 1, 2023 at 12:43=E2=80=AFPM Xu Kuohai <xukuohai@huaweicloud.com=
> wrote:
>
> From: Xu Kuohai <xukuohai@huawei.com>
>
> When BPF_TRAMP_F_CALL_ORIG is set, bpf trampoline uses BLR to jump
> back to the instruction next to call site to call the patched function.
> For BTI-enabled kernel, the instruction next to call site is usually
> PACIASP, in this case, it's safe to jump back with BLR. But when
> the call site is not followed by a PACIASP or bti, a BTI exception
> is triggered.
>
> Here is a fault log:
>
>  Unhandled 64-bit el1h sync exception on CPU0, ESR 0x0000000034000002 -- =
BTI
>  CPU: 0 PID: 263 Comm: test_progs Tainted: GF
>  Hardware name: linux,dummy-virt (DT)
>  pstate: 40400805 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=3D-c)
>  pc : bpf_fentry_test1+0xc/0x30
>  lr : bpf_trampoline_6442573892_0+0x48/0x1000
>  sp : ffff80000c0c3a50
>  x29: ffff80000c0c3a90 x28: ffff0000c2e6c080 x27: 0000000000000000
>  x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000050
>  x23: 0000000000000000 x22: 0000ffffcfd2a7f0 x21: 000000000000000a
>  x20: 0000ffffcfd2a7f0 x19: 0000000000000000 x18: 0000000000000000
>  x17: 0000000000000000 x16: 0000000000000000 x15: 0000ffffcfd2a7f0
>  x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
>  x11: 0000000000000000 x10: ffff80000914f5e4 x9 : ffff8000082a1528
>  x8 : 0000000000000000 x7 : 0000000000000000 x6 : 0101010101010101
>  x5 : 0000000000000000 x4 : 00000000fffffff2 x3 : 0000000000000001
>  x2 : ffff8001f4b82000 x1 : 0000000000000000 x0 : 0000000000000001
>  Kernel panic - not syncing: Unhandled exception
>  CPU: 0 PID: 263 Comm: test_progs Tainted: GF
>  Hardware name: linux,dummy-virt (DT)
>  Call trace:
>   dump_backtrace+0xec/0x144
>   show_stack+0x24/0x7c
>   dump_stack_lvl+0x8c/0xb8
>   dump_stack+0x18/0x34
>   panic+0x1cc/0x3ec
>   __el0_error_handler_common+0x0/0x130
>   el1h_64_sync_handler+0x60/0xd0
>   el1h_64_sync+0x78/0x7c
>   bpf_fentry_test1+0xc/0x30
>   bpf_fentry_test1+0xc/0x30
>   bpf_prog_test_run_tracing+0xdc/0x2a0
>   __sys_bpf+0x438/0x22a0
>   __arm64_sys_bpf+0x30/0x54
>   invoke_syscall+0x78/0x110
>   el0_svc_common.constprop.0+0x6c/0x1d0
>   do_el0_svc+0x38/0xe0
>   el0_svc+0x30/0xd0
>   el0t_64_sync_handler+0x1ac/0x1b0
>   el0t_64_sync+0x1a0/0x1a4
>  Kernel Offset: disabled
>  CPU features: 0x0000,00034c24,f994fdab
>  Memory Limit: none
>
> And the instruction next to call site of bpf_fentry_test1 is ADD,
> not PACIASP:
>
> <bpf_fentry_test1>:
>         bti     c
>         nop
>         nop
>         add     w0, w0, #0x1
>         paciasp
>
> For bpf prog, jit always puts a PACIASP after call site for BTI-enabled
> kernel, so there is no problem.
>
> To fix it, replace BLR with RET to bypass the branch target check.
>
> Fixes: efc9909fdce0 ("bpf, arm64: Add bpf trampoline for arm64")
> Reported-by: Florent Revest <revest@chromium.org>
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>

Thank you Xu! It does fix the BTI Oops I observed :)

Tested-by: Florent Revest <revest@chromium.org>
Acked-by: Florent Revest <revest@chromium.org>

> ---
>  arch/arm64/net/bpf_jit.h      | 4 ++++
>  arch/arm64/net/bpf_jit_comp.c | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h
> index a6acb94ea3d6..c2edadb8ec6a 100644
> --- a/arch/arm64/net/bpf_jit.h
> +++ b/arch/arm64/net/bpf_jit.h
> @@ -281,4 +281,8 @@
>  /* DMB */
>  #define A64_DMB_ISH aarch64_insn_gen_dmb(AARCH64_INSN_MB_ISH)
>
> +/* ADR */
> +#define A64_ADR(Rd, offset) \
> +       aarch64_insn_gen_adr(0, offset, Rd, AARCH64_INSN_ADR_TYPE_ADR)
> +
>  #endif /* _BPF_JIT_H */
> diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.=
c
> index 62f805f427b7..b26da8efa616 100644
> --- a/arch/arm64/net/bpf_jit_comp.c
> +++ b/arch/arm64/net/bpf_jit_comp.c
> @@ -1900,7 +1900,8 @@ static int prepare_trampoline(struct jit_ctx *ctx, =
struct bpf_tramp_image *im,
>                 restore_args(ctx, args_off, nargs);
>                 /* call original func */
>                 emit(A64_LDR64I(A64_R(10), A64_SP, retaddr_off), ctx);
> -               emit(A64_BLR(A64_R(10)), ctx);
> +               emit(A64_ADR(A64_LR, AARCH64_INSN_SIZE * 2), ctx);
> +               emit(A64_RET(A64_R(10)), ctx);
>                 /* store return value */
>                 emit(A64_STR64I(A64_R(0), A64_SP, retval_off), ctx);
>                 /* reserve a nop for bpf_tramp_image_put */
> --
> 2.30.2
>