| Message ID | 20221018135920.726360-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fixes for dynptr | expand |
On 10/18, 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. > 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). > Finally, 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. > Finally, since check_func_arg_reg_off is meant to be generic, move > dynptr specific check into process_dynptr_func. > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 55 +++++++++++++++---- > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > 2 files changed, 44 insertions(+), 13 deletions(-) > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index a49b95c1af1b..a8c277e51d63 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5654,6 +5654,14 @@ int process_dynptr_func(struct bpf_verifier_env > *env, int regno, > return -EFAULT; > } > + /* CONST_PTR_TO_DYNPTR has fixed and variable offset as zero, ensured by > + * check_func_arg_reg_off, so this is only needed for PTR_TO_STACK. > + */ > + if (reg->off % BPF_REG_SIZE) { > + verbose(env, "cannot pass in dynptr at an offset\n"); > + return -EINVAL; > + } This is what I'm missing here and in the original code as well, maybe you can clarify? "if (reg->off & BPF_REG_SIZE)" here vs "if (reg->off)" below. What's the difference? > + > /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a > * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): > * > @@ -5672,6 +5680,7 @@ int process_dynptr_func(struct bpf_verifier_env > *env, int regno, > * destroyed, including mutation of the memory it points > * to. > */ > + > if (arg_type & MEM_UNINIT) { > if (!is_dynptr_reg_valid_uninit(env, reg)) { > verbose(env, "Dynptr has to be an uninitialized dynptr\n"); > @@ -5983,14 +5992,37 @@ int check_func_arg_reg_off(struct > bpf_verifier_env *env, > enum bpf_reg_type type = reg->type; > bool fixed_off_ok = false; > - switch ((u32)type) { > - /* Pointer types where reg 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"); > + /* 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 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) > + goto check_type; > + /* Going straight to check 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; > } > - fallthrough; > + goto check; > + } > +check_type: > + switch ((u32)type) { > + /* Pointer types where both fixed and variable reg offset is explicitly > + * allowed: */ > + case PTR_TO_STACK: > case PTR_TO_PACKET: > case PTR_TO_PACKET_META: > case PTR_TO_MAP_KEY: > @@ -6001,12 +6033,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_ALLOC_MEM) > - return 0; > - break; > + return 0; > /* All the rest must be rejected, except PTR_TO_BTF_ID which allows > * fixed offset. > */ > @@ -6023,12 +6050,16 @@ int check_func_arg_reg_off(struct > bpf_verifier_env *env, > /* 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. > + * > + * var_off always must be 0 for PTR_TO_BTF_ID, hence we still > + * need to do checks instead of returning. > */ > fixed_off_ok = true; > break; > default: > break; > } > +check: > return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); > } > 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.0
On Wed, Oct 19, 2022 at 03:25:21AM IST, sdf@google.com wrote: > On 10/18, 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. > > > 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). > > > Finally, 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. > > > Finally, since check_func_arg_reg_off is meant to be generic, move > > dynptr specific check into process_dynptr_func. > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/verifier.c | 55 +++++++++++++++---- > > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > > 2 files changed, 44 insertions(+), 13 deletions(-) > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index a49b95c1af1b..a8c277e51d63 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -5654,6 +5654,14 @@ int process_dynptr_func(struct bpf_verifier_env > > *env, int regno, > > return -EFAULT; > > } > > > + /* CONST_PTR_TO_DYNPTR has fixed and variable offset as zero, ensured by > > + * check_func_arg_reg_off, so this is only needed for PTR_TO_STACK. > > + */ > > + if (reg->off % BPF_REG_SIZE) { > > + verbose(env, "cannot pass in dynptr at an offset\n"); > > + return -EINVAL; > > + } > > This is what I'm missing here and in the original code as well, maybe you > can clarify? > > "if (reg->off & BPF_REG_SIZE)" here vs "if (reg->off)" below. What's the > difference? > That second one happens earlier in check_func_arg_reg_off, this check happens later. Usually when we have release arguments, we want pointer to object unmodified. So the fixed and variable offset must be 0. The check_func_arg_reg_off checks ensure that. But PTR_TO_STACK in case of dynptr release functions point to the dynptr object on the stack which has to be released. In this case fp will have some fixed offset. So we make an exception for it and fallback to normal checks for PTR_TO_STACK. Later when we come here, we reach the function for two kinds of registers, CONST_PTR_TO_DYNPTR and PTR_TO_STACK. PTR_TO_STACK reg->off must be aligned to 8-byte alignment since we want to find stack slot index (each representing 8 byte slot) of the dynptr to operate on it. For CONST_PTR_TO_DYNPTR it directly points to dynptr with 0 offset, which check_func_arg_reg_off already ensures for it. Note that this reg->off check is actually broken, the correct one is in patch 6 which takes into account the variable offset. You can consider check_func_arg_reg_off to only do high level checks which are common for all helpers, and later processing builds upon those guarantees and does further checking.
On Tue, Oct 18, 2022 at 6:59 AM 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. > > 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). > > Finally, 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. > > Finally, since check_func_arg_reg_off is meant to be generic, move > dynptr specific check into process_dynptr_func. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 55 +++++++++++++++---- > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > 2 files changed, 44 insertions(+), 13 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index a49b95c1af1b..a8c277e51d63 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -5654,6 +5654,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > return -EFAULT; > } > > + /* CONST_PTR_TO_DYNPTR has fixed and variable offset as zero, ensured by > + * check_func_arg_reg_off, so this is only needed for PTR_TO_STACK. > + */ > + if (reg->off % BPF_REG_SIZE) { > + verbose(env, "cannot pass in dynptr at an offset\n"); > + return -EINVAL; > + } > + Imo, this logic belongs more in check_func_arg_reg_off(). It's cleaner to me to have all the logic for reg->off checking consolidated in one place. > /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a > * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): > * > @@ -5672,6 +5680,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > * destroyed, including mutation of the memory it points > * to. > */ > + > if (arg_type & MEM_UNINIT) { > if (!is_dynptr_reg_valid_uninit(env, reg)) { > verbose(env, "Dynptr has to be an uninitialized dynptr\n"); > @@ -5983,14 +5992,37 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > enum bpf_reg_type type = reg->type; > bool fixed_off_ok = false; > > - switch ((u32)type) { > - /* Pointer types where reg 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"); > + /* 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 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) Do we need the "arg_type_is_dynptr(arg_type)" part? I think just "if (type == PTR_TO_STACK)" suffices since any release args on the stack will be at some fp offset. > + goto check_type; I think this logic is a lot simpler to read: if (arg_type_is_release(arg_type)) { if (type != PTR_TO_STACK) { if (reg->off) { verbose(env, "R%d must have zero offset..."); return -EINVAL; } return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); } } > + /* Going straight to check 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; > } > - fallthrough; > + goto check; I think it's cleaner here to just "return __check_ptr_off_reg(env, reg, regno, fixed_off_ok);" instead of adding the goto check. > + } > +check_type: > + switch ((u32)type) { btw I don't think we need this (u32) cast. type is an enum bpf_reg_type, which is by default a u32. > + /* Pointer types where both fixed and variable reg offset is explicitly > + * allowed: */ > + case PTR_TO_STACK: > case PTR_TO_PACKET: > case PTR_TO_PACKET_META: > case PTR_TO_MAP_KEY: > @@ -6001,12 +6033,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_ALLOC_MEM) > - return 0; > - break; > + return 0; > /* All the rest must be rejected, except PTR_TO_BTF_ID which allows > * fixed offset. > */ We should also remove the "if (arg_type_is_release(arg_type) && reg->off)" code in the PTR_TO_BTF_ID case. > @@ -6023,12 +6050,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > /* 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. > + * > + * var_off always must be 0 for PTR_TO_BTF_ID, hence we still > + * need to do checks instead of returning. > */ > fixed_off_ok = true; > break; > default: > break; > } > +check: > return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); > } > > 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.0 >
On Tue, Nov 08, 2022 at 04:47:08AM IST, Joanne Koong wrote: > On Tue, Oct 18, 2022 at 6:59 AM 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. > > > > 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). > > > > Finally, 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. > > > > Finally, since check_func_arg_reg_off is meant to be generic, move > > dynptr specific check into process_dynptr_func. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/verifier.c | 55 +++++++++++++++---- > > .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- > > 2 files changed, 44 insertions(+), 13 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index a49b95c1af1b..a8c277e51d63 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -5654,6 +5654,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > return -EFAULT; > > } > > > > + /* CONST_PTR_TO_DYNPTR has fixed and variable offset as zero, ensured by > > + * check_func_arg_reg_off, so this is only needed for PTR_TO_STACK. > > + */ > > + if (reg->off % BPF_REG_SIZE) { > > + verbose(env, "cannot pass in dynptr at an offset\n"); > > + return -EINVAL; > > + } > > + > > Imo, this logic belongs more in check_func_arg_reg_off(). It's cleaner > to me to have all the logic for reg->off checking consolidated in one > place. > I think this alignment requirement is specific to dynptr, so it should be here. My idea with this patch was to only force offset rules per register type that don't harcode any assumptions about what each helper does with that register type. Each ARG_TYPE_* can then further build upon what this function guarantees about the register type. e.g. PTR_TO_MAP_VALUE doesn't have restriction to have constant var_off, but a lot of helpers require it to be const. It wouldn't make sense to move their helper specific offset checks to this function. Same reasoning here. > > /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a > > * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): > > * > > @@ -5672,6 +5680,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, > > * destroyed, including mutation of the memory it points > > * to. > > */ > > + > > if (arg_type & MEM_UNINIT) { > > if (!is_dynptr_reg_valid_uninit(env, reg)) { > > verbose(env, "Dynptr has to be an uninitialized dynptr\n"); > > @@ -5983,14 +5992,37 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, > > enum bpf_reg_type type = reg->type; > > bool fixed_off_ok = false; > > > > - switch ((u32)type) { > > - /* Pointer types where reg 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"); > > + /* 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 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) > > Do we need the "arg_type_is_dynptr(arg_type)" part? I think just "if > (type == PTR_TO_STACK)" suffices since any release args on the stack > will be at some fp offset. > Just being more careful here. We can drop it once we have more cases, but it's better IMO to be more restrictive by default to prevent things from slipping through. In the future there will be more helpers that work similar to dynptr (e.g. initializing a bpf_list_head on stack), then we can abstract them behind a common check. > > + goto check_type; > > I think this logic is a lot simpler to read: > > if (arg_type_is_release(arg_type)) { > if (type != PTR_TO_STACK) { > if (reg->off) { > verbose(env, "R%d must have zero offset..."); > return -EINVAL; > } > return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); > } > } > Sure, I can rewrite it like this. > > + /* Going straight to check 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; > > } > > - fallthrough; > > + goto check; > > I think it's cleaner here to just "return __check_ptr_off_reg(env, > reg, regno, fixed_off_ok);" instead of adding the goto check. > > > + } > > +check_type: > > + switch ((u32)type) { > > btw I don't think we need this (u32) cast. type is an enum > bpf_reg_type, which is by default a u32. > nit: it's an int by default. I've found clang complaining when you switch over an enum type and cases contain non-enum constants (like type | flag). For clarity I'll declare the variable as u32. > > + /* Pointer types where both fixed and variable reg offset is explicitly > > + * allowed: */ > > + case PTR_TO_STACK: > > case PTR_TO_PACKET: > > case PTR_TO_PACKET_META: > > case PTR_TO_MAP_KEY: > > @@ -6001,12 +6033,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_ALLOC_MEM) > > - return 0; > > - break; > > + return 0; > > /* All the rest must be rejected, except PTR_TO_BTF_ID which allows > > * fixed offset. > > */ > > We should also remove the "if (arg_type_is_release(arg_type) && > reg->off)" code in the PTR_TO_BTF_ID case. > Ack.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a49b95c1af1b..a8c277e51d63 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5654,6 +5654,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, return -EFAULT; } + /* CONST_PTR_TO_DYNPTR has fixed and variable offset as zero, ensured by + * check_func_arg_reg_off, so this is only needed for PTR_TO_STACK. + */ + if (reg->off % BPF_REG_SIZE) { + verbose(env, "cannot pass in dynptr at an offset\n"); + return -EINVAL; + } + /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): * @@ -5672,6 +5680,7 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, * destroyed, including mutation of the memory it points * to. */ + if (arg_type & MEM_UNINIT) { if (!is_dynptr_reg_valid_uninit(env, reg)) { verbose(env, "Dynptr has to be an uninitialized dynptr\n"); @@ -5983,14 +5992,37 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, enum bpf_reg_type type = reg->type; bool fixed_off_ok = false; - switch ((u32)type) { - /* Pointer types where reg 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"); + /* 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 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) + goto check_type; + /* Going straight to check 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; } - fallthrough; + goto check; + } +check_type: + switch ((u32)type) { + /* Pointer types where both fixed and variable reg offset is explicitly + * allowed: */ + case PTR_TO_STACK: case PTR_TO_PACKET: case PTR_TO_PACKET_META: case PTR_TO_MAP_KEY: @@ -6001,12 +6033,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_ALLOC_MEM) - return 0; - break; + return 0; /* All the rest must be rejected, except PTR_TO_BTF_ID which allows * fixed offset. */ @@ -6023,12 +6050,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, /* 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. + * + * var_off always must be 0 for PTR_TO_BTF_ID, hence we still + * need to do checks instead of returning. */ fixed_off_ok = true; break; default: break; } +check: return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); } 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. 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). Finally, 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. Finally, since check_func_arg_reg_off is meant to be generic, move dynptr specific check into process_dynptr_func. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 55 +++++++++++++++---- .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- 2 files changed, 44 insertions(+), 13 deletions(-)