Re: [PATCH bpf-next v2 1/2] bpf: Enforce gotox targets against subprog bounds

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

On 26/06/13 05:33PM, Nuoqi Gui wrote:
> CFG construction records the modeled gotox target set in
> insn_aux_data->jt. It includes INSN_ARRAY maps based on whether the map
> target is in the current subprog. check_indirect_jump() later validates and
> follows the current PTR_TO_INSN register's actual INSN_ARRAY map. The
> verifier does not check that targets copied from that map stay inside the
> same subprog as the gotox instruction.
> 
> This lets one gotox instruction observe two different INSN_ARRAY maps. CFG
> can select a map whose target is in the current subprog. Another path to
> the same gotox can carry a PTR_TO_INSN value from a map whose target points
> at a different subprog. The verifier then accepts a cross-subprog edge that
> CFG construction did not allow for this gotox instruction.

Functionally, the patch is ok. But IMHO the commit message is too complex.
Please consider making it shorter, if it will be respinned.

> On x86, gotox becomes a raw indirect jump in the JIT image. Accepting a
> target outside the gotox subprog can enter another subprog without a
> matching BPF call frame and crash when executed. Validation observed a GPF
> in bpf_test_run().
> 
> Fix this by requiring every target copied from the actual PTR_TO_INSN
> map to stay within the subprog that contains the current gotox instruction.

'the subprog that contains the current gotox instruction' -> 'the calling subprog'?

> Reject the program before pushing verifier states for any cross-subprog
> target.

Is this sentence needed at all? ^

> 
> Fixes: 493d9e0d6083 ("bpf, x86: add support for indirect jumps")
> Signed-off-by: Nuoqi Gui <gnq25@mails.tsinghua.edu.cn>
> ---
>  kernel/bpf/verifier.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index eb46a81a8c51..98d3fa2f162a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17145,9 +17145,11 @@ static int indirect_jump_min_max_index(struct bpf_verifier_env *env,
>  static int check_indirect_jump(struct bpf_verifier_env *env, struct bpf_insn *insn)
>  {
>  	struct bpf_verifier_state *other_branch;
> +	struct bpf_subprog_info *subprog;
>  	struct bpf_reg_state *dst_reg;
>  	struct bpf_map *map;
>  	u32 min_index, max_index;
> +	int subprog_start, subprog_end;
>  	int err = 0;
>  	int n;
>  	int i;
> @@ -17188,6 +17190,25 @@ static int check_indirect_jump(struct bpf_verifier_env *env, struct bpf_insn *in
>  		return -EINVAL;
>  	}
>  
> +	subprog = bpf_find_containing_subprog(env, env->insn_idx);
> +	if (verifier_bug_if(!subprog, env,
> +			    "gotox insn %d is outside subprog bounds\n",
> +			    env->insn_idx))
> +		return -EFAULT;
> +	subprog_start = subprog->start;
> +	subprog_end = (subprog + 1)->start;
> +
> +	for (i = 0; i < n; i++) {
> +		u32 target = env->gotox_tmp_buf->items[i];
> +
> +		if (target < subprog_start || target >= subprog_end) {
> +			verbose(env,
> +				"gotox target %u from map id=%d is outside subprog [%d,%d)\n",
> +				target, map->id, subprog_start, subprog_end);
> +			return -EINVAL;
> +		}
> +	}
> +

This could have been a helper to share code with create_jt(),
but looks small enough to keep it as is.

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

>  	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],
> 
> -- 
> 2.34.1
>