Re: [bpf-next v8 4/5] bpf, x86: Emit ENDBR for indirect jump targets

[Anton Protopopov]

On 26/03/09 10:00PM, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@xxxxxxxxxx>
> 
> On CPUs that support CET/IBT, the indirect jump selftest triggers
> a kernel panic because the indirect jump targets lack ENDBR
> instructions.
> 
> To fix it, emit an ENDBR instruction to each indirect jump target. Since
> the ENDBR instruction shifts the position of original jited instructions,
> fix the instruction address calculation wherever the addresses are used.
> 
> For reference, below is a sample panic log.
> 
>  Missing ENDBR: bpf_prog_2e5f1c71c13ac3e0_big_jump_table+0x97/0xe1
>  ------------[ cut here ]------------
>  kernel BUG at arch/x86/kernel/cet.c:133!
>  Oops: invalid opcode: 0000 [#1] SMP NOPTI
> 
>  ...
> 
>   ? 0xffffffffc00fb258
>   ? bpf_prog_2e5f1c71c13ac3e0_big_jump_table+0x97/0xe1
>   bpf_prog_test_run_syscall+0x110/0x2f0
>   ? fdget+0xba/0xe0
>   __sys_bpf+0xe4b/0x2590
>   ? __kmalloc_node_track_caller_noprof+0x1c7/0x680
>   ? bpf_prog_test_run_syscall+0x215/0x2f0
>   __x64_sys_bpf+0x21/0x30
>   do_syscall_64+0x85/0x620
>   ? bpf_prog_test_run_syscall+0x1e2/0x2f0
> 
> Fixes: 493d9e0d6083 ("bpf, x86: add support for indirect jumps")
> Signed-off-by: Xu Kuohai <xukuohai@xxxxxxxxxx>
> ---
>  arch/x86/net/bpf_jit_comp.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index b95f23ad1093..251dff1cd8e4 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1649,8 +1649,8 @@ static int emit_spectre_bhb_barrier(u8 **pprog, u8 *ip,
>  	return 0;
>  }
>  
> -static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
> -		  int oldproglen, struct jit_context *ctx, bool jmp_padding)
> +static int do_jit(struct bpf_verifier_env *env, struct bpf_prog *bpf_prog, int *addrs, u8 *image,
> +		  u8 *rw_image, int oldproglen, struct jit_context *ctx, bool jmp_padding)
>  {
>  	bool tail_call_reachable = bpf_prog->aux->tail_call_reachable;
>  	struct bpf_insn *insn = bpf_prog->insnsi;
> @@ -1663,7 +1663,7 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image
>  	void __percpu *priv_stack_ptr;
>  	int i, excnt = 0;
>  	int ilen, proglen = 0;
> -	u8 *prog = temp;
> +	u8 *ip, *prog = temp;
>  	u32 stack_depth;
>  	int err;
>  
> @@ -1734,6 +1734,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image
>  				dst_reg = X86_REG_R9;
>  		}
>  
> +#ifdef CONFIG_X86_KERNEL_IBT
> +		if (bpf_insn_is_indirect_target(env, bpf_prog, i - 1))
> +			EMIT_ENDBR();
> +#endif
> +
> +		ip = image + addrs[i - 1] + (prog - temp);
> +
>  		switch (insn->code) {
>  			/* ALU */
>  		case BPF_ALU | BPF_ADD | BPF_X:
> @@ -2440,8 +2447,6 @@ st:			if (is_imm8(insn->off))
>  
>  			/* call */
>  		case BPF_JMP | BPF_CALL: {
> -			u8 *ip = image + addrs[i - 1];
> -
>  			func = (u8 *) __bpf_call_base + imm32;
>  			if (src_reg == BPF_PSEUDO_CALL && tail_call_reachable) {
>  				LOAD_TAIL_CALL_CNT_PTR(stack_depth);
> @@ -2465,7 +2470,8 @@ st:			if (is_imm8(insn->off))
>  			if (imm32)
>  				emit_bpf_tail_call_direct(bpf_prog,
>  							  &bpf_prog->aux->poke_tab[imm32 - 1],
> -							  &prog, image + addrs[i - 1],
> +							  &prog,
> +							  ip,
>  							  callee_regs_used,
>  							  stack_depth,
>  							  ctx);
> @@ -2474,7 +2480,7 @@ st:			if (is_imm8(insn->off))
>  							    &prog,
>  							    callee_regs_used,
>  							    stack_depth,
> -							    image + addrs[i - 1],
> +							    ip,
>  							    ctx);
>  			break;
>  
> @@ -2639,7 +2645,7 @@ st:			if (is_imm8(insn->off))
>  			break;
>  
>  		case BPF_JMP | BPF_JA | BPF_X:
> -			emit_indirect_jump(&prog, insn->dst_reg, image + addrs[i - 1]);
> +			emit_indirect_jump(&prog, insn->dst_reg, ip);
>  			break;
>  		case BPF_JMP | BPF_JA:
>  		case BPF_JMP32 | BPF_JA:
> @@ -2729,8 +2735,6 @@ st:			if (is_imm8(insn->off))
>  			ctx->cleanup_addr = proglen;
>  			if (bpf_prog_was_classic(bpf_prog) &&
>  			    !ns_capable_noaudit(&init_user_ns, CAP_SYS_ADMIN)) {
> -				u8 *ip = image + addrs[i - 1];
> -
>  				if (emit_spectre_bhb_barrier(&prog, ip, bpf_prog))
>  					return -EINVAL;
>  			}
> @@ -3791,7 +3795,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_verifier_env *env, struct bpf_pr
>  	for (pass = 0; pass < MAX_PASSES || image; pass++) {
>  		if (!padding && pass >= PADDING_PASSES)
>  			padding = true;
> -		proglen = do_jit(prog, addrs, image, rw_image, oldproglen, &ctx, padding);
> +		proglen = do_jit(env, prog, addrs, image, rw_image, oldproglen, &ctx, padding);
>  		if (proglen <= 0) {
>  out_image:
>  			image = NULL;
> -- 
> 2.47.3

Reviewed-by: Anton Protopopov <a.s.protopopov@xxxxxxxxx>