| Message ID | 20221207204141.308952-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 184c9bdb8f65d9f909b3a089a83bc0b0f1e1ea4c |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr refactorings | expand |
On Thu, Dec 08, 2022 at 02:11:38AM +0530, 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. commit log is obsolete. I cleaned it up s/ARG_PTR_TO_ALLOC_MEM/ARG_PTR_TO_RINGBUF_MEM/ > This commit aims to streamline these special cases and instead leave > other things up to argument type specific code to handle. The function > will be restrictive by default, and cover all possible cases when > OBJ_RELEASE is set, without having to update the function again (and > missing to do that being a bug). > > 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 and MEM_RINGBUF here. > 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). > > 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. > > This is not specific to ARG_PTR_TO_DYNPTR though, we will need to make > this exception for any future object on the stack that needs to be > released. In this sense, PTR_TO_STACK as a candidate for object on stack > argument is a special case for release offset checks, and they need to > be done by the helper releasing the object on stack. > > Since the check has been lifted above all register type checks, remove > the duplicated check that is being done for PTR_TO_BTF_ID. > > Acked-by: Joanne Koong <joannelkoong@gmail.com> > Acked-by: David Vernet <void@manifault.com> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 64 +++++++++++-------- > tools/testing/selftests/bpf/verifier/calls.c | 2 +- > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > 3 files changed, 41 insertions(+), 27 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index fdbaf22cdaf2..cadcf0233326 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6269,11 +6269,38 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > const struct bpf_reg_state *reg, int regno, > enum bpf_arg_type arg_type) > { > - enum bpf_reg_type type = reg->type; > - bool fixed_off_ok = false; > + u32 type = reg->type; > > - switch ((u32)type) { > - /* Pointer types where reg offset is explicitly allowed: */ > + /* When referenced register is passed to release function, its fixed > + * offset must be 0. > + * > + * We will check arg_type_is_release reg has ref_obj_id when storing > + * meta->release_regno. > + */ > + if (arg_type_is_release(arg_type)) { > + /* ARG_PTR_TO_DYNPTR with OBJ_RELEASE is a bit special, as it > + * may not directly point to the object being released, but to > + * dynptr pointing to such object, which might be at some offset > + * on the stack. In that case, we simply to fallback to the > + * default handling. > + */ > + if (!arg_type_is_dynptr(arg_type) || type != PTR_TO_STACK) { also removed one indent, inverted above and added direct 'return 0' I know that it's not exactly the same, but together with patch 5 it makes it cleaner and this special case (that David didn't like) doesn't look as horrible anymore. > + /* Doing check_ptr_off_reg check for the offset will > + * catch this because fixed_off_ok is false, but > + * checking here allows us to give the user a better > + * error message. > + */ > + if (reg->off) { > + verbose(env, "R%d must have zero offset when passed to release func or trusted arg to kfunc\n", > + regno); > + return -EINVAL; > + } > + return __check_ptr_off_reg(env, reg, regno, false); > + } > + /* Fallback to default handling */ No 'fallback to default' either. It wasn't easy to follow this logic. And applied. See how it looks now. Overall looks like great cleanup. Thanks!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index fdbaf22cdaf2..cadcf0233326 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6269,11 +6269,38 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno, enum bpf_arg_type arg_type) { - enum bpf_reg_type type = reg->type; - bool fixed_off_ok = false; + u32 type = reg->type; - switch ((u32)type) { - /* Pointer types where reg offset is explicitly allowed: */ + /* When referenced register is passed to release function, its fixed + * offset must be 0. + * + * We will check arg_type_is_release reg has ref_obj_id when storing + * meta->release_regno. + */ + if (arg_type_is_release(arg_type)) { + /* ARG_PTR_TO_DYNPTR with OBJ_RELEASE is a bit special, as it + * may not directly point to the object being released, but to + * dynptr pointing to such object, which might be at some offset + * on the stack. In that case, we simply to fallback to the + * default handling. + */ + if (!arg_type_is_dynptr(arg_type) || type != PTR_TO_STACK) { + /* Doing check_ptr_off_reg check for the offset will + * catch this because fixed_off_ok is false, but + * checking here allows us to give the user a better + * error message. + */ + if (reg->off) { + verbose(env, "R%d must have zero offset when passed to release func or trusted arg to kfunc\n", + regno); + return -EINVAL; + } + return __check_ptr_off_reg(env, reg, regno, false); + } + /* Fallback to default handling */ + } + 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"); @@ -6290,12 +6317,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, case PTR_TO_BUF: case PTR_TO_BUF | MEM_RDONLY: case SCALAR_VALUE: - /* Some of the argument types nevertheless require a - * zero register offset. - */ - if (base_type(arg_type) != ARG_PTR_TO_RINGBUF_MEM) - return 0; - break; + return 0; /* All the rest must be rejected, except PTR_TO_BTF_ID which allows * fixed offset. */ @@ -6305,24 +6327,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, case PTR_TO_BTF_ID | MEM_RCU: case PTR_TO_BTF_ID | MEM_ALLOC | PTR_TRUSTED: /* When referenced PTR_TO_BTF_ID is passed to release function, - * it's fixed offset must be 0. In the other cases, fixed offset - * can be non-zero. + * its fixed offset must be 0. In the other cases, fixed offset + * can be non-zero. This was already checked above. So pass + * fixed_off_ok as true to allow fixed offset for all other + * cases. var_off always must be 0 for PTR_TO_BTF_ID, hence we + * still need to do checks instead of returning. */ - if (arg_type_is_release(arg_type) && reg->off) { - verbose(env, "R%d must have zero offset when passed to release func\n", - regno); - return -EINVAL; - } - /* For arg is release pointer, fixed_off_ok must be false, but - * we already checked and rejected reg->off != 0 above, so set - * to true to allow fixed offset for all other cases. - */ - fixed_off_ok = true; - break; + return __check_ptr_off_reg(env, reg, regno, true); default: - break; + return __check_ptr_off_reg(env, reg, regno, false); } - return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); } static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c index 3193915c5ee6..babcec123251 100644 --- a/tools/testing/selftests/bpf/verifier/calls.c +++ b/tools/testing/selftests/bpf/verifier/calls.c @@ -76,7 +76,7 @@ }, .prog_type = BPF_PROG_TYPE_SCHED_CLS, .result = REJECT, - .errstr = "arg#0 expected pointer to ctx, but got PTR", + .errstr = "R1 must have zero offset when passed to release func or trusted arg to kfunc", .fixup_kfunc_btf_id = { { "bpf_kfunc_call_test_pass_ctx", 2 }, }, diff --git a/tools/testing/selftests/bpf/verifier/ringbuf.c b/tools/testing/selftests/bpf/verifier/ringbuf.c index 84838feba47f..92e3f6a61a79 100644 --- a/tools/testing/selftests/bpf/verifier/ringbuf.c +++ b/tools/testing/selftests/bpf/verifier/ringbuf.c @@ -28,7 +28,7 @@ }, .fixup_map_ringbuf = { 1 }, .result = REJECT, - .errstr = "dereference of modified ringbuf_mem ptr R1", + .errstr = "R1 must have zero offset when passed to release func", }, { "ringbuf: invalid reservation offset 2",