Re: [PATCH bpf-next v2 1/1] bpf: Simplify checking size of helper accesses

[Eduard Zingerman <eddyz87@gmail.com>]

On Sat, 2023-12-16 at 20:06 -0500, Andrei Matei wrote:
[...]

> (*) Besides standing to reason that the checks for a bigger size access
> are a super-set of the checks for a smaller size access, I have also
> mechanically verified this by reading the code for all types of
> pointers. I could convince myself that it's true for all but
> PTR_TO_BTF_ID (check_ptr_to_btf_access). There, simply looking
> line-by-line does not immediately prove what we want. If anyone has any
> qualms, let me know.

check_help_mem_access() is a bit obfuscated :)
After staring at it for a bit I have a question regarding
check_ptr_to_btf_access():
- it can call btf_struct_access(),
  which in can call btf_struct_walk(),
  which has the following check:

		if (btf_type_is_ptr(mtype)) {
			const struct btf_type *stype, *t;
			enum bpf_type_flag tmp_flag = 0;
			u32 id;

			if (msize != size || off != moff) {
				bpf_log(log,
					"cannot access ptr member %s with moff %u in struct %s with off %u size %u\n",
					mname, moff, tname, off, size);
				return -EACCES;
			}

- previously this code was executed twice, for size 0 and for size
  umax_value of the size register;
- now this code is executed only for umax_value of the size register;
- is it possible that with size 0 this code could have reported error
  -EACCESS error, which would be missed now?
  
Except for the question above I don't see any issues,
but check_help_mem_access() has many sub-cases,
so I might have missed something.

Also a few nits below.

[...]

> @@ -7256,6 +7256,65 @@ static int check_helper_mem_access(struct bpf_verifier_env *env, int regno,
>  	}
>  }
>  
> +/* Helper function for logging an error about an invalid attempt to perform a
> + * (possibly) zero-sized memory access. The pointer being dereferenced is in
> + * register @ptr_regno, and the size of the access is in register @size_regno.
> + * The size register is assumed to either be a constant zero or have a zero lower
> + * bound.
> + *
> + * Logs a message like:
> + * invalid zero-size read. Size comes from R2=0. Attempting to dereference *map_value R1: off=[0,4] value_size=48
> + */
> +static void log_zero_size_access_err(struct bpf_verifier_env *env,
> +			      int ptr_regno,
> +			      int size_regno)
> +{
> +	struct bpf_reg_state *ptr_reg = &cur_regs(env)[ptr_regno];
> +	struct bpf_reg_state *size_reg = &cur_regs(env)[size_regno];
> +	const bool size_is_const = tnum_is_const(size_reg->var_off);
> +	const char *ptr_type_str = reg_type_str(env, ptr_reg->type);
> +	/* allocate a few buffers to be used as parts of the error message */
> +	char size_range_buf[64] = {0}, max_size_buf[64] = {0}, off_buf[64] = {0};
> +	s64 min_off, max_off;

Nit: empty is needed here

[...]

>  /* verify arguments to helpers or kfuncs consisting of a pointer and an access
>   * size.
>   *
> @@ -7268,6 +7327,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
>  			      struct bpf_call_arg_meta *meta)
>  {
>  	int err;
> +	const bool size_is_const = tnum_is_const(reg->var_off);

Nit: please swap definitions to get the "reverse Christmas tree":

    const bool size_is_const = tnum_is_const(reg->var_off);
    int err;

>  
>  	/* This is used to refine r0 return value bounds for helpers
>  	 * that enforce this value as an upper bound on return values.
> @@ -7282,7 +7342,7 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
>  	/* The register is SCALAR_VALUE; the access check
>  	 * happens using its boundaries.
>  	 */
> -	if (!tnum_is_const(reg->var_off))
> +	if (!size_is_const)
>  		/* For unprivileged variable accesses, disable raw
>  		 * mode so that the program is required to
>  		 * initialize all the memory that the helper could
> @@ -7296,12 +7356,9 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
>  		return -EACCES;
>  	}
>  
> -	if (reg->umin_value == 0) {
> -		err = check_helper_mem_access(env, regno - 1, 0,
> -					      zero_size_allowed,
> -					      meta);
> -		if (err)
> -			return err;
> +	if (reg->umin_value == 0 && !zero_size_allowed) {
> +		log_zero_size_access_err(env, regno-1, regno);
> +		return -EACCES;
>  	}
>  
>  	if (reg->umax_value >= BPF_MAX_VAR_SIZ) {
> @@ -7309,9 +7366,21 @@ static int check_mem_size_reg(struct bpf_verifier_env *env,
>  			regno);
>  		return -EACCES;
>  	}
> +	/* If !zero_size_allowed, we already checked that umin_value > 0, so
> +	 * umax_value should also be > 0.
> +	 */
> +	if (reg->umax_value == 0 && !zero_size_allowed) {
> +		verbose(env, "verifier bug: !zero_size_allowed should have been handled already\n");
> +		return -EFAULT;
> +	}
>  	err = check_helper_mem_access(env, regno - 1,
>  				      reg->umax_value,
> -				      zero_size_allowed, meta);
> +				      /* zero_size_allowed: we asserted above that umax_value is
> +				       * not zero if !zero_size_allowed, so we don't need any
> +				       * further checks.
> +				       */
> +				      true ,
                          ^
Nit: extra space ---------'

> +				      meta);
>  	if (!err)
>  		err = mark_chain_precision(env, regno);
>  	return err;

[...]