Re: [bpf-next v8 3/5] bpf: Add helper to detect indirect jump targets

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/03/09 10:00PM, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> Introduce helper bpf_insn_is_indirect_target to check whether a BPF
> instruction is an indirect jump target.
> 
> Since the verifier knows which instructions are indirect jump targets,
> add a new flag indirect_target to struct bpf_insn_aux_data to mark
> them. The verifier sets this flag when verifing an indirect jump target
> instruction, and the helper checks it to determine whether an
> instruction is an indirect jump target.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  include/linux/bpf.h          |  2 ++
>  include/linux/bpf_verifier.h |  9 +++++----
>  kernel/bpf/core.c            |  9 +++++++++
>  kernel/bpf/verifier.c        | 18 ++++++++++++++++++
>  4 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 05b34a6355b0..90760e250865 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1541,6 +1541,8 @@ bool bpf_has_frame_pointer(unsigned long ip);
>  int bpf_jit_charge_modmem(u32 size);
>  void bpf_jit_uncharge_modmem(u32 size);
>  bool bpf_prog_has_trampoline(const struct bpf_prog *prog);
> +bool bpf_insn_is_indirect_target(const struct bpf_verifier_env *env, const struct bpf_prog *prog,
> +				 int insn_idx);
>  #else
>  static inline int bpf_trampoline_link_prog(struct bpf_tramp_link *link,
>  					   struct bpf_trampoline *tr,
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 090aa26d1c98..6431b94746cf 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -578,16 +578,17 @@ struct bpf_insn_aux_data {
>  
>  	/* below fields are initialized once */
>  	unsigned int orig_idx; /* original instruction index */
> -	bool jmp_point;
> -	bool prune_point;
> +	u32 jmp_point:1;
> +	u32 prune_point:1;
>  	/* ensure we check state equivalence and save state checkpoint and
>  	 * this instruction, regardless of any heuristics
>  	 */
> -	bool force_checkpoint;
> +	u32 force_checkpoint:1;
>  	/* true if instruction is a call to a helper function that
>  	 * accepts callback function as a parameter.
>  	 */
> -	bool calls_callback;
> +	u32 calls_callback:1;
> +	u32 indirect_target:1; /* if it is an indirect jump target */
>  	/*
>  	 * CFG strongly connected component this instruction belongs to,
>  	 * zero if it is a singleton SCC.
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 88a1834030a0..ccc618a50103 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1480,6 +1480,15 @@ int bpf_jit_blind_constants(struct bpf_verifier_env *env)
>  	prog->blinded = 1;
>  	return 0;
>  }
> +
> +bool bpf_insn_is_indirect_target(const struct bpf_verifier_env *env, const struct bpf_prog *prog,
> +				 int insn_idx)
> +{
> +	if (!env)
> +		return false;
> +	insn_idx += prog->aux->subprog_start;
> +	return env->insn_aux_data[insn_idx].indirect_target;
> +}
>  #endif /* CONFIG_BPF_JIT */
>  
>  /* Base function for offset calculation. Needs to go into .text section,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fc3ba380b74a..8d8e349f98cd 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4022,6 +4022,11 @@ static bool is_jmp_point(struct bpf_verifier_env *env, int insn_idx)
>  	return env->insn_aux_data[insn_idx].jmp_point;
>  }
>  
> +static void mark_indirect_target(struct bpf_verifier_env *env, int idx)
> +{
> +	env->insn_aux_data[idx].indirect_target = true;
> +}
> +
>  #define LR_FRAMENO_BITS	3
>  #define LR_SPI_BITS	6
>  #define LR_ENTRY_BITS	(LR_SPI_BITS + LR_FRAMENO_BITS + 1)
> @@ -21082,12 +21087,14 @@ static int check_indirect_jump(struct bpf_verifier_env *env, struct bpf_insn *in
>  	}
>  
>  	for (i = 0; i < n - 1; i++) {
> +		mark_indirect_target(env, env->gotox_tmp_buf->items[i]);
>  		other_branch = push_stack(env, env->gotox_tmp_buf->items[i],
>  					  env->insn_idx, env->cur_state->speculative);
>  		if (IS_ERR(other_branch))
>  			return PTR_ERR(other_branch);
>  	}
>  	env->insn_idx = env->gotox_tmp_buf->items[n-1];
> +	mark_indirect_target(env, env->insn_idx);
>  	return 0;
>  }
>  
> @@ -22013,6 +22020,17 @@ static void adjust_insn_aux_data(struct bpf_verifier_env *env,
>  		data[i].seen = old_seen;
>  		data[i].zext_dst = insn_has_def32(insn + i);
>  	}
> +
> +	/* The indirect_target flag of the original instruction was moved to the last of the
> +	 * new instructions by the above memmove and memset, but the indirect jump target is
> +	 * actually the first instruction, so move it back. This also matches with the behavior
> +	 * of bpf_insn_array_adjust(), which preserves xlated_off to point to the first new
> +	 * instruction.
> +	 */
> +	if (data[off + cnt - 1].indirect_target) {
> +		data[off].indirect_target = 1;
> +		data[off + cnt - 1].indirect_target = 0;
> +	}
>  }
>  
>  static void adjust_subprog_starts(struct bpf_verifier_env *env, u32 off, u32 len)
> -- 
> 2.47.3
> 

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