Re: [PATCH bpf-next v1 5/7] bpf: Move PTR_TO_STACK alignment check to process_dynptr_func

[David Vernet <void@manifault.com>]

On Tue, Nov 15, 2022 at 05:31:28AM +0530, Kumar Kartikeya Dwivedi wrote:
> After previous commit, we are minimizing helper specific assumptions
> from check_func_arg_reg_off, making it generic, and offloading checks
> for a specific argument type to their respective functions called after
> check_func_arg_reg_off has been called.

What's the point of check_func_arg_reg_off() if helpers have to check
offsets after it's been called? Also, in [0], there's now logic in
check_func_arg_reg_off() which checks for OBJ_RELEASE arg types, so
there's still a precedent for looking at arg types there. IMO it's
actually less confusing to just push as much offset checking as possible
into one place.

[0]: https://lore.kernel.org/all/20221115000130.1967465-5-memxor@gmail.com/

> This allows relying on a consistent set of guarantees after that call
> and then relying on them in code that deals with registers for each
> argument type later. This is in line with how process_spin_lock,
> process_timer_func, process_kptr_func check reg->var_off to be constant.
> The same reasoning is used here to move the alignment check into
> process_dynptr_func. Note that it also needs to check for constant
> var_off, and accumulate the constant var_off when computing the spi in
> get_spi, but that fix will come in later changes.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  kernel/bpf/verifier.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 34e67d04579b..fd292f762d53 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5774,6 +5774,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
>  		return -EFAULT;
>  	}
>  
> +	/* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to
> +	 * check_func_arg_reg_off's logic. We only need to check offset
> +	 * alignment for PTR_TO_STACK.
> +	 */
> +	if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) {
> +		verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off);
> +		return -EINVAL;
> +	}

As alluded to above, I personally think it's more confusing to have this
check in process_dynptr_func(). On the one hand you have
check_func_arg_reg_off() which verifies that a register has the correct
offset, but then here we have to check for the register offset for
PTR_TO_STACK dynptrs specifically? Wouldn't it be better to try and push
as much of the offset-checking complexity into one place as possible?

>  	/* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a
>  	 * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
>  	 *
> @@ -6125,11 +6133,6 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
>  	switch (type) {
>  	/* Pointer types where both fixed and variable offset is explicitly allowed: */
>  	case PTR_TO_STACK:
> -		if (arg_type_is_dynptr(arg_type) && reg->off % BPF_REG_SIZE) {
> -			verbose(env, "cannot pass in dynptr at an offset\n");
> -			return -EINVAL;
> -		}
> -		fallthrough;
>  	case PTR_TO_PACKET:
>  	case PTR_TO_PACKET_META:
>  	case PTR_TO_MAP_KEY:
> -- 
> 2.38.1
>