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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Nov 21, 2022 at 12:40:13AM +0530, Kumar Kartikeya Dwivedi wrote:
> 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'.

We certainly don't want to make check_func_arg_reg_off() a monster
function, but I think we can do better in terms of making the verifier
consistent and easier to reason about by pushing more logic into it.

My view (subject to change upon learning new context I may be missing)
is that the job of check_func_arg_reg_off() should be to map all
(reg_type x arg_type) combinations to checking the correct offset,
likely by calling __check_ptr_off_reg() with the correct arguments. The
signature / implementation of __check_ptr_off_reg() may need to change
for that to happen, and we may need to e.g. also leverage
__check_mem_access() to accommodate the mem register types.

Yes, doing this may cause check_func_arg_reg_off() to have a potentially
very large switch statement (though there are other ways to address
that), but isn't that preferable to having to read through hundreds or
thousands of lines of verifier code to convince yourself that an offset
was correctly determined for every possible (register x arg_type)?
Having all of that contained in one, well-defined spot seems much
simpler. Please let me know if I've grossly misunderstood something, or
am missing important / relevant context.

> Consider process_kptr_func, it requires var_off to be constant. Same for

IMO that check should certainly go in check_func_arg_reg_off().
__check_ptr_off_reg() already checks for tnum_is_const(reg->var_off) in
__check_ptr_off_reg(), and check_func_arg_reg_off() has all the
information it needs to encapsulate the logic for that check.

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

Personally I think "drawing the line" is the wrong way to think about
it. We need to decide what role the function plays, and generalize it in
a way that's consistent and clear. IMO its role at a high level should
be, "The verify arg / register offsets step in the verifier". If you
break that encapsulation, it becomes much more difficult to build a
consistent mental model of what the verifier is doing. Note that this
applies to other verification steps as well, such as e.g. verifying
types, verifying proper refcounts, etc. Perhaps I'm grossly naive for
thinking this is possible? Please let me know if you think that's the
case.

Anyways, it might not be possible to aggregate all logic for checking
reg->off into the function in the codebase as it exists today, but it
seems like a desirable end-state, and it feels like a step backwards to
start selectively moving reg->off checking out of
check_func_arg_reg_off() and into arg / helper specific functions.

> 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 */

Hmmm, but these are all just reg_type, right? Why are we checking
OBJ_RELEASE in check_func_arg_reg_off() if that's the case?

> 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.

I definitely agree that there may be helper-specific verification that
needs to be done. We're talking about arg_type and reg_type, though.
Those aren't specific to an _individual_ helper (though yes, of course
arg_types are specific to whatever _sets_ of helpers take them, such as
e.g. dynptrs).

If we go with the approach of having individual arg types or sets of
helpers verify offsets, I think that still needs to be generalized so
that it's happening in a single place. This would involve something
like:

1. Having check_func_arg_reg_off() as it exists today be renamed to
check_func_reg_off(), and be solely responsible for checking reg_type.

2. Update check_func_arg_reg_off() to contain all the logic which does
actual arg type checking, possibly calling out to one of many possible
helper functions depending on the arg type.

My main issue is really just the fact that all of this logic is
scattered throughout the verifier.

> 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@xxxxxxxxx/
> >
> > > 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@xxxxxxxxx>
> > > ---
> > >  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.

If we're going to do that, IMO we should just remove the arg_type
parameter from the function altogether. Having specific arg_type logic
in the function feels artificial.

> 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.

Agreed that this is the crux of the issue. I think I've said my piece so
I won't reply directly to this point here.

> 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.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux