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

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

On Fri, Nov 18, 2022 at 05:19:27AM IST, David Vernet wrote:
> 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.
>

I think you need to define 'as much offset checking'.

Consider process_kptr_func, it requires var_off to be constant. Same for
bpf_timer, bpf_spin_lock, bpf_list_head, bpf_list_node, etc. They take
PTR_TO_MAP_VALUE, PTR_TO_BTF_ID, PTR_TO_BTF_ID | MEM_ALLOC. Should we move all
of that into check_func_arg_reg_off?

Some argument types like ARG_PTR_TO_MEM are ok with variable offset, should that
exception go in this function as well?

Where do you draw the line here in terms of what that function does?

IMO, there are a certain set of properties that check_func_arg_reg_off provides,
you could say that if each register type was a class, then the checks there
would be what you would do while constructing them on calling:

PtrToStack(off, var_off /* can be const or variable */)
PtrToMapValue(off, var_off /* can be const or variable */)
PtrToBtfId(off /* must be >= 0 */) /* no var_off */

How they get used by each helper and what further checks each helper needs to do
on them based on the arg_type should be done at a later stage when the specific
argument type is processed.

Agreed that, it's not perfect, with the odd case for PTR_TO_STACK having non-0
reg->off for OBJ_RELEASE. But IMO once you realise it makes no sense to release
PTR_TO_STACK and that PTR_TO_STACK actually points to the real pointer being
released, it needs to be handled specially.

> [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?
>

I'm trying to make a split between 'offset correct for the reg->type in
general', and 'offset correct for the reg->type when when passed as an argument
for arg_type'. I think the latter is specific and different for each case and
thus belongs inside the case ARG_TYPE_* block of each of those.

Anyhow, all of this is not to reject your point, but to say that if we're
keeping that check in check_func_arg_reg_off for dynptr, let's also examine why
we should/shouldn't move checks for other arg_types inside it as well, and
whether the end result is going to be better than this.

In that case, atleast to me, it doesn't make sense to check reg->off %
BPF_REG_SIZE for ARG_PTR_TO_DYNPTR while leaving out other arg types.