On Thu, Dec 08, 2022 at 02:11:38AM +0530, 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.
commit log is obsolete.
I cleaned it up s/ARG_PTR_TO_ALLOC_MEM/ARG_PTR_TO_RINGBUF_MEM/
> This commit aims to streamline these special cases and instead leave
> other things up to argument type specific code to handle. The function
> will be restrictive by default, and cover all possible cases when
> OBJ_RELEASE is set, without having to update the function again (and
> missing to do that being a bug).
>
> 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
and MEM_RINGBUF here.
> 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).
>
> 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.
>
> This is not specific to ARG_PTR_TO_DYNPTR though, we will need to make
> this exception for any future object on the stack that needs to be
> released. In this sense, PTR_TO_STACK as a candidate for object on stack
> argument is a special case for release offset checks, and they need to
> be done by the helper releasing the object on stack.
>
> Since the check has been lifted above all register type checks, remove
> the duplicated check that is being done for PTR_TO_BTF_ID.
>
> Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> Acked-by: David Vernet <void@xxxxxxxxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 64 +++++++++++--------
> tools/testing/selftests/bpf/verifier/calls.c | 2 +-
> .../testing/selftests/bpf/verifier/ringbuf.c | 2 +-
> 3 files changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index fdbaf22cdaf2..cadcf0233326 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6269,11 +6269,38 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
> const struct bpf_reg_state *reg, int regno,
> enum bpf_arg_type arg_type)
> {
> - enum bpf_reg_type type = reg->type;
> - bool fixed_off_ok = false;
> + u32 type = reg->type;
>
> - switch ((u32)type) {
> - /* Pointer types where reg offset is explicitly allowed: */
> + /* When referenced register is passed to release function, its 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 with OBJ_RELEASE 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) {
also removed one indent, inverted above and added direct 'return 0'
I know that it's not exactly the same, but together with patch 5
it makes it cleaner and this special case (that David didn't like)
doesn't look as horrible anymore.
> + /* Doing check_ptr_off_reg check for the offset 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 or trusted arg to kfunc\n",
> + regno);
> + return -EINVAL;
> + }
> + return __check_ptr_off_reg(env, reg, regno, false);
> + }
> + /* Fallback to default handling */
No 'fallback to default' either.
It wasn't easy to follow this logic.
And applied.
See how it looks now.
Overall looks like great cleanup. Thanks!
