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

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, Oct 19, 2022 at 03:25:21AM IST, sdf@google.com wrote:
> 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?
>

That second one happens earlier in check_func_arg_reg_off, this check happens
later.

Usually when we have release arguments, we want pointer to object unmodified.
So the fixed and variable offset must be 0. The check_func_arg_reg_off checks
ensure that. But PTR_TO_STACK in case of dynptr release functions point to the
dynptr object on the stack which has to be released.

In this case fp will have some fixed offset. So we make an exception for it and
fallback to normal checks for PTR_TO_STACK.

Later when we come here, we reach the function for two kinds of registers,
CONST_PTR_TO_DYNPTR and PTR_TO_STACK. PTR_TO_STACK reg->off must be aligned
to 8-byte alignment since we want to find stack slot index (each representing 8
byte slot) of the dynptr to operate on it.

For CONST_PTR_TO_DYNPTR it directly points to dynptr with 0 offset, which
check_func_arg_reg_off already ensures for it.

Note that this reg->off check is actually broken, the correct one is in patch 6
which takes into account the variable offset.

You can consider check_func_arg_reg_off to only do high level checks which are
common for all helpers, and later processing builds upon those guarantees and
does further checking.