| Message ID | 20221115000130.1967465-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Dynptr refactorings | expand |
On Mon, Nov 14, 2022 at 4:01 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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. > > 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 > 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. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> Acked-by: Joanne Koong <joannelkoong@gmail.com> > --- > kernel/bpf/verifier.c | 62 ++++++++++++------- > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > 2 files changed, 39 insertions(+), 25 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index c484e632b0cd..34e67d04579b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6092,11 +6092,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, it's 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\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"); > @@ -6113,35 +6140,22 @@ 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_ALLOC_MEM) > - return 0; > - break; > + return 0; > /* All the rest must be rejected, except PTR_TO_BTF_ID which allows > * fixed offset. > */ > case PTR_TO_BTF_ID: > /* 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. > + * 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/ringbuf.c b/tools/testing/selftests/bpf/verifier/ringbuf.c > index b64d33e4833c..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 alloc_mem ptr R1", > + .errstr = "R1 must have zero offset when passed to release func", > }, > { > "ringbuf: invalid reservation offset 2", > -- > 2.38.1 >
On Tue, Nov 15, 2022 at 05:31:27AM +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. > > 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 > with non-zero offset to other helper functions, which makes sense. Agreed -- just out of curiosity do you know what the rationale was for it disallowing offsets in the first place? > 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. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 62 ++++++++++++------- > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > 2 files changed, 39 insertions(+), 25 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index c484e632b0cd..34e67d04579b 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6092,11 +6092,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, it's fixed s/it's/its > + * 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. > + */ I personally am not a fan of this. We'd now have an entirely separate branch of logic for most OBJ_RELEASE args, but then we fall through to default handling for only PTR_TO_STACK specifically? That kind of tactical complexity / readability tax is unfortunate, and adds up quickly. It's already gotten pretty pervasive in the verifier, and I personally would prefer to see us moving away from adding small one-off conditional branches like this in the verifier when possible. It seems to me like the problem we're trying to solve is that both arg type and register type are relevant when detecting whether a register is allowed to have nonzero offsets. It would be ideal if we could encode some of this at build-time, similar to how we statically define and compare compatible_reg_types in check_reg_type(). Is there some way for us to do this for allowing offsets? Like maybe we can hoist some of the build logic for how we define compatible_reg_types into a separate table-like header file that can be included in multiple places to construct statically-defined, constant arrays where all of this logic is encoded? These check-offset / check-type functions could then really just be lookups into these const arrays, and e.g. enabling new arg types for register types could be captured by adding an entry to that header-file table. Maybe that's overkill for this specific example, but I really feel like we need to start to try and move away from these one-off conditional branches which collectively create a lot of complexity, and make it difficult to reason about high-level correctness in the verifier. To be clear: I won't block your change on this, but I'd like to hear what you think about it as an idea, and I personally would really like to see the verifier moving in that direction in general. > + 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\n", > + regno); > + return -EINVAL; > + } Why is this check necessary? To get a better error message in the verifier if the register offset is nonzero? If so, IMO I don't think that's a good enough reason to completely circumvent calling __check_ptr_off_reg(). Not for correctness, but because it's __check_ptr_off_reg()'s job to look at the register offset (that's what the fixed_off_ok arg is for after all), and adding one-off checks like which duplicate checks in other functions, it ends up ballooning the code and complexity in the verifier. If we want to have a different message for testing the offset being zero depending on the arg type, that's where it belongs. > + 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"); > @@ -6113,35 +6140,22 @@ 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_ALLOC_MEM) > - return 0; Doesn't this result in a change of behavior beyond just verifying an offset of 0? Before, in addition to checking offset 0, we were also verifying that e.g. reg->off was nonnegative for ARG_PTR_TO_ALLOC_MEM. Now we only do that if it's OBJ_RELEASE. I'm actually not following why we wouldn't want to invoke __check_ptr_offset() for any of these other register types. > - break; > + return 0; > /* All the rest must be rejected, except PTR_TO_BTF_ID which allows > * fixed offset. > */ > case PTR_TO_BTF_ID: > /* When referenced PTR_TO_BTF_ID is passed to release function, > * it's fixed offset must be 0. In the other cases, fixed offset Not your change, but while you're here could you please fix s/it's/its > - * can be non-zero. > + * 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/ringbuf.c b/tools/testing/selftests/bpf/verifier/ringbuf.c > index b64d33e4833c..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 alloc_mem ptr R1", > + .errstr = "R1 must have zero offset when passed to release func", > }, > { > "ringbuf: invalid reservation offset 2", > -- > 2.38.1 >
On Fri, Nov 18, 2022 at 05:12:07AM IST, David Vernet wrote: > On Tue, Nov 15, 2022 at 05:31:27AM +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. > > > > 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 > > with non-zero offset to other helper functions, which makes sense. > > Agreed -- just out of curiosity do you know what the rationale was for > it disallowing offsets in the first place? > 64620e0a1e71 ("bpf: Fix out of bounds access for ringbuf helpers") It predates OBJ_RELEASE's addition. > > 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. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/verifier.c | 62 ++++++++++++------- > > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > > 2 files changed, 39 insertions(+), 25 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index c484e632b0cd..34e67d04579b 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -6092,11 +6092,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, it's fixed > > s/it's/its > Ack. > > + * 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. > > + */ > > I personally am not a fan of this. We'd now have an entirely separate > branch of logic for most OBJ_RELEASE args, but then we fall through to > default handling for only PTR_TO_STACK specifically? That kind of > tactical complexity / readability tax is unfortunate, and adds up > quickly. It's already gotten pretty pervasive in the verifier, and I > personally would prefer to see us moving away from adding small one-off > conditional branches like this in the verifier when possible. I think that's because PTR_TO_STACK is a bit special. Unlike normal register types which you get from acquire functions, you instead create some kind of resource on the stack in this case. So the actual pointer that is to be released (dynptr wrapping ringbuf reservation) is actually on the stack, and unlike the normal case there is an indirection. This will be the same with more cases in the future (e.g. moving a list_head to the stack, which you will have to release, etc.). > > It seems to me like the problem we're trying to solve is that both arg > type and register type are relevant when detecting whether a register is > allowed to have nonzero offsets. It would be ideal if we could encode > some of this at build-time, similar to how we statically define and > compare compatible_reg_types in check_reg_type(). Is there some way for > us to do this for allowing offsets? Like maybe we can hoist some of the > build logic for how we define compatible_reg_types into a separate > table-like header file that can be included in multiple places to > construct statically-defined, constant arrays where all of this logic is > encoded? These check-offset / check-type functions could then really > just be lookups into these const arrays, and e.g. enabling new arg types > for register types could be captured by adding an entry to that > header-file table. > It would be easier to understand this if you can share it as an RFC/PoC (i.e. if you determine it helps and is an improvement in terms of readability). > Maybe that's overkill for this specific example, but I really feel like > we need to start to try and move away from these one-off conditional > branches which collectively create a lot of complexity, and make it > difficult to reason about high-level correctness in the verifier. > I totally agree. The code is complicated. > To be clear: I won't block your change on this, but I'd like to hear > what you think about it as an idea, and I personally would really like > to see the verifier moving in that direction in general. > I'm all for simplifications. I already tried dropping a few special cases, but I was not able to figure out how we can capture the requirement for objects on stack being released any better than this. Feel free to provide more suggestions though, based on the above. > > + if (!arg_type_is_dynptr(arg_type) || type != PTR_TO_STACK) { So technically this will be called arg_type_is_object_on_stack once we have more cases in the future. > > + /* 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\n", > > + regno); > > + return -EINVAL; > > + } > > Why is this check necessary? To get a better error message in the > verifier if the register offset is nonzero? If so, IMO I don't think > that's a good enough reason to completely circumvent calling > __check_ptr_off_reg(). But it's not circumvented, it's called after this check? > Not for correctness, but because it's > __check_ptr_off_reg()'s job to look at the register offset (that's what > the fixed_off_ok arg is for after all), and adding one-off checks like > which duplicate checks in other functions, it ends up ballooning the > code and complexity in the verifier. If we want to have a different > message for testing the offset being zero depending on the arg type, > that's where it belongs. > The problem is that it's also invoked in other places outside of check_func_arg_reg_off where that bool is_release_arg parameter would make no sense. I don't have a strong opinion on moving it inside the function though, just sharing my thoughts. > > + 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"); > > @@ -6113,35 +6140,22 @@ 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_ALLOC_MEM) > > - return 0; > > Doesn't this result in a change of behavior beyond just verifying an > offset of 0? Before, in addition to checking offset 0, we were also > verifying that e.g. reg->off was nonnegative for ARG_PTR_TO_ALLOC_MEM. Since that translates to PTR_TO_MEM, __check_mem_access handles the negative offset case (all memory types do, and PTR_TO_BTF_ID interprets off as u32). PTR_TO_STACK has to explicitly permit reg->off < 0. > Now we only do that if it's OBJ_RELEASE. I'm actually not following why > we wouldn't want to invoke __check_ptr_offset() for any of these other > register types. > Because all of them having non-zero reg->off, reg->var_off is totally ok. The point of this function is to reject all other register types. > > - break; > > + return 0; > > /* All the rest must be rejected, except PTR_TO_BTF_ID which allows > > * fixed offset. > > */ > > case PTR_TO_BTF_ID: > > /* When referenced PTR_TO_BTF_ID is passed to release function, > > * it's fixed offset must be 0. In the other cases, fixed offset > > Not your change, but while you're here could you please fix s/it's/its > Will do.
On Mon, Nov 21, 2022 at 12:11:37AM +0530, Kumar Kartikeya Dwivedi wrote: > On Fri, Nov 18, 2022 at 05:12:07AM IST, David Vernet wrote: > > On Tue, Nov 15, 2022 at 05:31:27AM +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. > > > > > > 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 > > > with non-zero offset to other helper functions, which makes sense. > > > > Agreed -- just out of curiosity do you know what the rationale was for > > it disallowing offsets in the first place? > > > > 64620e0a1e71 ("bpf: Fix out of bounds access for ringbuf helpers") > It predates OBJ_RELEASE's addition. Thanks [...] TL;DR: going to ack this for now so the set can land before in plenty of time before the 6.1 release. Thanks again for the cleanup + fixes in this set. Left some other comments below that I'd like to discuss and then possibly address at some point down the road. Acked-by: David Vernet <void@manifault.com> > > > + * 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. > > > + */ > > > > I personally am not a fan of this. We'd now have an entirely separate > > branch of logic for most OBJ_RELEASE args, but then we fall through to > > default handling for only PTR_TO_STACK specifically? That kind of > > tactical complexity / readability tax is unfortunate, and adds up > > quickly. It's already gotten pretty pervasive in the verifier, and I > > personally would prefer to see us moving away from adding small one-off > > conditional branches like this in the verifier when possible. > > I think that's because PTR_TO_STACK is a bit special. Unlike normal register > types which you get from acquire functions, you instead create some kind of > resource on the stack in this case. So the actual pointer that is to be released > (dynptr wrapping ringbuf reservation) is actually on the stack, and unlike the > normal case there is an indirection. > > This will be the same with more cases in the future (e.g. moving a list_head to > the stack, which you will have to release, etc.). Yeah, I understand why it's this way. For now I think it's fine, but IMO the fact that we branch out like this for different register types is a sign that we need to add some more indirection here to clarify things a bit. As you reasonably requested below, once I have more time on my hands (probably not until 2023), I'm happy to explore this and send out an RFC patch set. Alexei mentioned that Daniel had been considering spending some time cleaning up the verifier as well, so I'd be curious to chat with him and see what he's been thinking. > > It seems to me like the problem we're trying to solve is that both arg > > type and register type are relevant when detecting whether a register is > > allowed to have nonzero offsets. It would be ideal if we could encode > > some of this at build-time, similar to how we statically define and > > compare compatible_reg_types in check_reg_type(). Is there some way for > > us to do this for allowing offsets? Like maybe we can hoist some of the > > build logic for how we define compatible_reg_types into a separate > > table-like header file that can be included in multiple places to > > construct statically-defined, constant arrays where all of this logic is > > encoded? These check-offset / check-type functions could then really > > just be lookups into these const arrays, and e.g. enabling new arg types > > for register types could be captured by adding an entry to that > > header-file table. > > > > It would be easier to understand this if you can share it as an RFC/PoC (i.e. if > you determine it helps and is an improvement in terms of readability). Totally reasonable request, I'll try to work on this once I get a few more things taken care of (probably won't be until 2023). > > > Maybe that's overkill for this specific example, but I really feel like > > we need to start to try and move away from these one-off conditional > > branches which collectively create a lot of complexity, and make it > > difficult to reason about high-level correctness in the verifier. > > > > I totally agree. The code is complicated. > > > To be clear: I won't block your change on this, but I'd like to hear > > what you think about it as an idea, and I personally would really like > > to see the verifier moving in that direction in general. > > > > I'm all for simplifications. I already tried dropping a few special cases, but I > was not able to figure out how we can capture the requirement for objects on > stack being released any better than this. > > Feel free to provide more suggestions though, based on the above. I don't have anything concrete to suggest for this revision. > > > + if (!arg_type_is_dynptr(arg_type) || type != PTR_TO_STACK) { > > So technically this will be called arg_type_is_object_on_stack once we have more > cases in the future. > > > > + /* 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\n", > > > + regno); > > > + return -EINVAL; > > > + } > > > > Why is this check necessary? To get a better error message in the > > verifier if the register offset is nonzero? If so, IMO I don't think > > that's a good enough reason to completely circumvent calling > > __check_ptr_off_reg(). > > But it's not circumvented, it's called after this check? Circumvent was probably the wrong word. What I meant was to say that this is unnecessarily short-circuiting the same offset check that is already present in __check_ptr_off_reg(). IMO, the purpose of check_func_arg_reg_off() is essentially to call __check_ptr_off_reg() with the correct parameters. It's mapping (reg x arg_type) -> what type of offset check should be performed for this register. I'm not convinced that adding another verifier message which complains about specifically release args is worth the extra cognitive overhead of someone reading this and trying to understand why there are multiple places where we're checking reg->off > 0. I think that is especially true given that some callers are setting OBJ_RELEASE to force a zero-offset check even though they're not actually release args, so the message may be misleading for some programs. Perhaps something like this is more generalizable: int err; bool fixed_off_ok = false; ... err = __check_ptr_off_reg(env, reg, regno, fixed_off_ok); if (err) verbose(env, "R%d reg with reg type %s, arg type %s had invalid offset %d\n", regno, reg_type_str(...), arg_type_str(...), reg->off); return err; This could be done at the bottom of the function so that all callers receive the same information in the event of an invalid reg offset. Apologies for the poor choice of using the word "circumvent" on the first email. > > > Not for correctness, but because it's > > __check_ptr_off_reg()'s job to look at the register offset (that's what > > the fixed_off_ok arg is for after all), and adding one-off checks like > > which duplicate checks in other functions, it ends up ballooning the > > code and complexity in the verifier. If we want to have a different > > message for testing the offset being zero depending on the arg type, > > that's where it belongs. > > > > The problem is that it's also invoked in other places outside of > check_func_arg_reg_off where that bool is_release_arg parameter would make no > sense. I agree with you on this -- adding arg_type to __check_ptr_off_reg() doesn't make sense (that's the point of check_func_arg_reg_off()). What do you think about my suggestion above instead? > I don't have a strong opinion on moving it inside the function though, just > sharing my thoughts. You're making reasonable points as well. Let me know if the above clarifies what I meant a bit. > > > + 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"); > > > @@ -6113,35 +6140,22 @@ 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_ALLOC_MEM) > > > - return 0; > > > > Doesn't this result in a change of behavior beyond just verifying an > > offset of 0? Before, in addition to checking offset 0, we were also > > verifying that e.g. reg->off was nonnegative for ARG_PTR_TO_ALLOC_MEM. > > Since that translates to PTR_TO_MEM, __check_mem_access handles the negative > offset case (all memory types do, and PTR_TO_BTF_ID interprets off as u32). > PTR_TO_STACK has to explicitly permit reg->off < 0. Ahh right, it's just PTR_TO_MEM. Thanks for clarifying. > > Now we only do that if it's OBJ_RELEASE. I'm actually not following why > > we wouldn't want to invoke __check_ptr_offset() for any of these other > > register types. > > > > Because all of them having non-zero reg->off, reg->var_off is totally ok. The > point of this function is to reject all other register types. Fair enough. I think part of my confusion here stemmed from the fact that these register types actually _do_ need to have their offsets verified, it just happens to be checked somewhere else (as you said, in __check_mem_access()). I wonder if at some point we want to refactor the verifier to bring __check_mem_access() into this function, and call it here for these pointer types? Or some other refactor that unifies where and how validation is done for different register types and arg types. IMO a somehwat significant part of the complexity in the verifier is the result of divergences like this between e.g. different reg types and arg types. Someone reading check_func_arg(), for example, will see that we call check_func_arg_reg_off(), and would arguably reasonably assume that the register offset has been verified by the time this is called. Anyways, I certainly don't mean to drag your patches through this digression. If and when we start to consider doing some more refactoring in the verifier, I think this is something we should keep in mind. In general this patch LGTM. As mentioned above, I'll sign off now so we can make sure this gets in before the 6.1 release, but I think it'd be good to revisit the verifier message thing I mentioned above once we all have a bit more bandwidth. Acked-by: David Vernet <void@manifault.com> [...]
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c484e632b0cd..34e67d04579b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6092,11 +6092,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, it's 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\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"); @@ -6113,35 +6140,22 @@ 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_ALLOC_MEM) - return 0; - break; + return 0; /* All the rest must be rejected, except PTR_TO_BTF_ID which allows * fixed offset. */ case PTR_TO_BTF_ID: /* 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. + * 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/ringbuf.c b/tools/testing/selftests/bpf/verifier/ringbuf.c index b64d33e4833c..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 alloc_mem ptr R1", + .errstr = "R1 must have zero offset when passed to release func", }, { "ringbuf: invalid reservation offset 2",
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. 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 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. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 62 ++++++++++++------- .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- 2 files changed, 39 insertions(+), 25 deletions(-)