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. [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? > /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a > * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): > * > @@ -6125,11 +6133,6 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > switch (type) { > /* Pointer types where both fixed and variable offset is explicitly allowed: */ > case PTR_TO_STACK: > - if (arg_type_is_dynptr(arg_type) && reg->off % BPF_REG_SIZE) { > - verbose(env, "cannot pass in dynptr at an offset\n"); > - return -EINVAL; > - } > - fallthrough; > case PTR_TO_PACKET: > case PTR_TO_PACKET_META: > case PTR_TO_MAP_KEY: > -- > 2.38.1 >
