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.
