Re: [PATCH bpf-next v1 04/13] bpf: Rework check_func_arg_reg_off

[sdf@google.com]

On 10/18, Kumar Kartikeya Dwivedi wrote:
> While check_func_arg_reg_off is the place which performs generic checks
> needed by various candidates of reg->type, there is some handling for
> special cases, like ARG_PTR_TO_DYNPTR, OBJ_RELEASE, and
> ARG_PTR_TO_ALLOC_MEM.

> This commit aims to streamline these special cases and instead leave
> other things up to argument type specific code to handle.

> This is done primarily for two reasons: associating back reg->type to
> its argument leaves room for the list getting out of sync when a new
> reg->type is supported by an arg_type.

> The other case is ARG_PTR_TO_ALLOC_MEM. The problem there is something
> we already handle, whenever a release argument is expected, it should
> be passed as the pointer that was received from the acquire function.
> Hence zero fixed and variable offset.

> There is nothing special about ARG_PTR_TO_ALLOC_MEM, where technically
> its target register type PTR_TO_MEM | MEM_ALLOC can already be passed
> with non-zero offset to other helper functions, which makes sense.

> Hence, lift the arg_type_is_release check for reg->off and cover all
> possible register types, instead of duplicating the same kind of check
> twice for current OBJ_RELEASE arg_types (alloc_mem and ptr_to_btf_id).

> Finally, for the release argument, arg_type_is_dynptr is the special
> case, where we go to actual object being freed through the dynptr, so
> the offset of the pointer still needs to allow fixed and variable offset
> and process_dynptr_func will verify them later for the release argument
> case as well.

> Finally, since check_func_arg_reg_off is meant to be generic, move
> dynptr specific check into process_dynptr_func.

> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>   kernel/bpf/verifier.c                         | 55 +++++++++++++++----
>   .../testing/selftests/bpf/verifier/ringbuf.c  |  2 +-
>   2 files changed, 44 insertions(+), 13 deletions(-)

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index a49b95c1af1b..a8c277e51d63 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5654,6 +5654,14 @@ int process_dynptr_func(struct bpf_verifier_env  
> *env, int regno,
>   		return -EFAULT;
>   	}

> +	/* CONST_PTR_TO_DYNPTR has fixed and variable offset as zero, ensured by
> +	 * check_func_arg_reg_off, so this is only needed for PTR_TO_STACK.
> +	 */
> +	if (reg->off % BPF_REG_SIZE) {
> +		verbose(env, "cannot pass in dynptr at an offset\n");
> +		return -EINVAL;
> +	}

This is what I'm missing here and in the original code as well, maybe you
can clarify?

"if (reg->off & BPF_REG_SIZE)" here vs "if (reg->off)" below. What's the
difference?

> +
>   	/* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a
>   	 * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*):
>   	 *
> @@ -5672,6 +5680,7 @@ int process_dynptr_func(struct bpf_verifier_env  
> *env, int regno,
>   	 *		 destroyed, including mutation of the memory it points
>   	 *		 to.
>   	 */
> +
>   	if (arg_type & MEM_UNINIT) {
>   		if (!is_dynptr_reg_valid_uninit(env, reg)) {
>   			verbose(env, "Dynptr has to be an uninitialized dynptr\n");
> @@ -5983,14 +5992,37 @@ int check_func_arg_reg_off(struct  
> bpf_verifier_env *env,
>   	enum bpf_reg_type type = reg->type;
>   	bool fixed_off_ok = false;

> -	switch ((u32)type) {
> -	/* Pointer types where reg 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");
> +	/* When referenced register is passed to release function, it's fixed
> +	 * offset must be 0.
> +	 *
> +	 * We will check arg_type_is_release reg has ref_obj_id when storing
> +	 * meta->release_regno.
> +	 */
> +	if (arg_type_is_release(arg_type)) {
> +		/* ARG_PTR_TO_DYNPTR is a bit special, as it may not directly
> +		 * point to the object being released, but to dynptr pointing
> +		 * to such object, which might be at some offset on the stack.
> +		 *
> +		 * In that case, we simply to fallback to the default handling.
> +		 */
> +		if (arg_type_is_dynptr(arg_type) && type == PTR_TO_STACK)
> +			goto check_type;
> +		/* Going straight to check will catch this because fixed_off_ok
> +		 * is false, but checking here allows us to give the user a
> +		 * better error message.
> +		 */
> +		if (reg->off) {
> +			verbose(env, "R%d must have zero offset when passed to release  
> func\n",
> +				regno);
>   			return -EINVAL;
>   		}
> -		fallthrough;
> +		goto check;
> +	}
> +check_type:
> +	switch ((u32)type) {
> +	/* Pointer types where both fixed and variable reg offset is explicitly
> +	 * allowed: */
> +	case PTR_TO_STACK:
>   	case PTR_TO_PACKET:
>   	case PTR_TO_PACKET_META:
>   	case PTR_TO_MAP_KEY:
> @@ -6001,12 +6033,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env  
> *env,
>   	case PTR_TO_BUF:
>   	case PTR_TO_BUF | MEM_RDONLY:
>   	case SCALAR_VALUE:
> -		/* Some of the argument types nevertheless require a
> -		 * zero register offset.
> -		 */
> -		if (base_type(arg_type) != ARG_PTR_TO_ALLOC_MEM)
> -			return 0;
> -		break;
> +		return 0;
>   	/* All the rest must be rejected, except PTR_TO_BTF_ID which allows
>   	 * fixed offset.
>   	 */
> @@ -6023,12 +6050,16 @@ int check_func_arg_reg_off(struct  
> bpf_verifier_env *env,
>   		/* For arg is release pointer, fixed_off_ok must be false, but
>   		 * we already checked and rejected reg->off != 0 above, so set
>   		 * to true to allow fixed offset for all other cases.
> +		 *
> +		 * var_off always must be 0 for PTR_TO_BTF_ID, hence we still
> +		 * need to do checks instead of returning.
>   		 */
>   		fixed_off_ok = true;
>   		break;
>   	default:
>   		break;
>   	}
> +check:
>   	return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);
>   }

> diff --git a/tools/testing/selftests/bpf/verifier/ringbuf.c  
> b/tools/testing/selftests/bpf/verifier/ringbuf.c
> index b64d33e4833c..92e3f6a61a79 100644
> --- a/tools/testing/selftests/bpf/verifier/ringbuf.c
> +++ b/tools/testing/selftests/bpf/verifier/ringbuf.c
> @@ -28,7 +28,7 @@
>   	},
>   	.fixup_map_ringbuf = { 1 },
>   	.result = REJECT,
> -	.errstr = "dereference of modified alloc_mem ptr R1",
> +	.errstr = "R1 must have zero offset when passed to release func",
>   },
>   {
>   	"ringbuf: invalid reservation offset 2",
> --
> 2.38.0