| Message ID | 20220301065745.1634848-5-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Fixes for bad PTR_TO_BTF_ID offset | expand |
On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote: > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc > always has its offset set to 0. While not a real problem now, there's a > very real possibility this will become a problem when more and more > kfuncs are exposed. > > Previous commits already protected against non-zero var_off. The case we > are concerned about now is when we have a type that can be returned by > acquire kfunc: > > struct foo { > int a; > int b; > struct bar b; > }; > > ... and struct bar is also a type that can be returned by another > acquire kfunc. > > Then, doing the following sequence: > > struct foo *f = bpf_get_foo(); // acquire kfunc > if (!f) > return 0; > bpf_put_bar(&f->b); // release kfunc > > ... would work with the current code, since the btf_struct_ids_match > takes reg->off into account for matching pointer type with release kfunc > argument type, but would obviously be incorrect, and most likely lead to > a kernel crash. A test has been included later to prevent regressions in > this area. > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/btf.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 7f6a0ae5028b..ba6845225b65 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > return -EINVAL; > } > > + if (is_kfunc) > + rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), > + BTF_KFUNC_TYPE_RELEASE, func_id); > /* check that BTF function arguments match actual types that the > * verifier sees. > */ > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > regno, reg->ref_obj_id, ref_obj_id); > return -EFAULT; > } > + /* Ensure that offset of referenced PTR_TO_BTF_ID is > + * always zero, when passed to release function. > + * var_off has already been checked to be 0 by > + * check_func_arg_reg_off. > + */ > + if (rel && reg->off) { Here is another reg->off check for PTR_TO_BTF_ID on top of the one 'check_func_arg_reg_off' added to the same function in patch 2. A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted considering the btf func does not need ARG_* to begin with. How about directly use the __check_ptr_off_reg() here instead of check_func_arg_reg_off()? Then patch 1 is not needed. Would something like this do the same thing (uncompiled code) ? diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7f6a0ae5028b..768cef4de4cc 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -5794,6 +5797,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, } } else if (is_kfunc && (reg->type == PTR_TO_BTF_ID || (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) { + bool fixed_off_ok = reg->type == PTR_TO_BTF_ID; const struct btf_type *reg_ref_t; const struct btf *reg_btf; const char *reg_ref_tname; @@ -5816,6 +5820,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, regno, reg->ref_obj_id, ref_obj_id); return -EFAULT; } + /* Ensure that offset of referenced PTR_TO_BTF_ID is + * always zero, when passed to release function. + * var_off has already been checked to be 0 by + * check_func_arg_reg_off. + */ + if (rel) + fixed_off_ok = false; ref_regno = regno; ref_obj_id = reg->ref_obj_id; } @@ -5824,6 +5835,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, reg_ref_id = *reg2btf_ids[base_type(reg->type)]; } + __check_ptr_off_reg(env, reg, regno, fixed_off_ok); reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id, ®_ref_id); reg_ref_tname = btf_name_by_offset(reg_btf,
On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote: > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote: > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc > > always has its offset set to 0. While not a real problem now, there's a > > very real possibility this will become a problem when more and more > > kfuncs are exposed. > > > > Previous commits already protected against non-zero var_off. The case we > > are concerned about now is when we have a type that can be returned by > > acquire kfunc: > > > > struct foo { > > int a; > > int b; > > struct bar b; > > }; > > > > ... and struct bar is also a type that can be returned by another > > acquire kfunc. > > > > Then, doing the following sequence: > > > > struct foo *f = bpf_get_foo(); // acquire kfunc > > if (!f) > > return 0; > > bpf_put_bar(&f->b); // release kfunc > > > > ... would work with the current code, since the btf_struct_ids_match > > takes reg->off into account for matching pointer type with release kfunc > > argument type, but would obviously be incorrect, and most likely lead to > > a kernel crash. A test has been included later to prevent regressions in > > this area. > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/btf.c | 15 +++++++++++++-- > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 7f6a0ae5028b..ba6845225b65 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > return -EINVAL; > > } > > > > + if (is_kfunc) > > + rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), > > + BTF_KFUNC_TYPE_RELEASE, func_id); > > /* check that BTF function arguments match actual types that the > > * verifier sees. > > */ > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > regno, reg->ref_obj_id, ref_obj_id); > > return -EFAULT; > > } > > + /* Ensure that offset of referenced PTR_TO_BTF_ID is > > + * always zero, when passed to release function. > > + * var_off has already been checked to be 0 by > > + * check_func_arg_reg_off. > > + */ > > + if (rel && reg->off) { > Here is another reg->off check for PTR_TO_BTF_ID on top of the > one 'check_func_arg_reg_off' added to the same function in patch 2. > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted > considering the btf func does not need ARG_* to begin with. > Right, arg_type doesn't really matter here (unless we start indicating in BTF we want to take ringbuf allocation directly without size parameter or getting size from BTF type). > How about directly use the __check_ptr_off_reg() here instead of > check_func_arg_reg_off()? Then patch 1 is not needed. > > Would something like this do the same thing (uncompiled code) ? > I should have included a link to the previous discussion, sorry about that: https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion Yes, this should also do the same thing, but the idea was to avoid keeping the same checks in multiple places. For now, there is only the special case of ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling, the former of which is currently not relevant for kfunc, but adding some future type and ensuring kfunc, and helper do the offset checks correctly just means updating check_func_arg_reg_off. reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special case. We should also do the same thing for BPF helpers, now that I look at it, but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it is quite possible to support it in more BPF helpers later and forget to prevent such case. So, it would be possible to move this check inside check_func_arg_reg_off, based on a new bool is_release_func parameter, and relying on the assumption that only one referenced register can be passed to helper or kfunc at a time (already enforced for both BPF helpers and kfuncs). Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we will do: fixed_off_ok = false; if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) fixed_off_ok = true; Again, given we can only pass one referenced reg, if we see release func and a reg with ref_obj_id, it is the one being released. In the end, it's more of a preference thing, if you feel strongly about it I can go with the __check_ptr_off_reg call too. > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 7f6a0ae5028b..768cef4de4cc 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -5794,6 +5797,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > } > } else if (is_kfunc && (reg->type == PTR_TO_BTF_ID || > (reg2btf_ids[base_type(reg->type)] && !type_flag(reg->type)))) { > + bool fixed_off_ok = reg->type == PTR_TO_BTF_ID; > const struct btf_type *reg_ref_t; > const struct btf *reg_btf; > const char *reg_ref_tname; > @@ -5816,6 +5820,13 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > regno, reg->ref_obj_id, ref_obj_id); > return -EFAULT; > } > + /* Ensure that offset of referenced PTR_TO_BTF_ID is > + * always zero, when passed to release function. > + * var_off has already been checked to be 0 by > + * check_func_arg_reg_off. > + */ > + if (rel) > + fixed_off_ok = false; > ref_regno = regno; > ref_obj_id = reg->ref_obj_id; > } > @@ -5824,6 +5835,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > reg_ref_id = *reg2btf_ids[base_type(reg->type)]; > } > > + __check_ptr_off_reg(env, reg, regno, fixed_off_ok); > reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id, > ®_ref_id); > reg_ref_tname = btf_name_by_offset(reg_btf, > -- Kartikeya
On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote: > On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote: > > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote: > > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc > > > always has its offset set to 0. While not a real problem now, there's a > > > very real possibility this will become a problem when more and more > > > kfuncs are exposed. > > > > > > Previous commits already protected against non-zero var_off. The case we > > > are concerned about now is when we have a type that can be returned by > > > acquire kfunc: > > > > > > struct foo { > > > int a; > > > int b; > > > struct bar b; > > > }; > > > > > > ... and struct bar is also a type that can be returned by another > > > acquire kfunc. > > > > > > Then, doing the following sequence: > > > > > > struct foo *f = bpf_get_foo(); // acquire kfunc > > > if (!f) > > > return 0; > > > bpf_put_bar(&f->b); // release kfunc > > > > > > ... would work with the current code, since the btf_struct_ids_match > > > takes reg->off into account for matching pointer type with release kfunc > > > argument type, but would obviously be incorrect, and most likely lead to > > > a kernel crash. A test has been included later to prevent regressions in > > > this area. > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > --- > > > kernel/bpf/btf.c | 15 +++++++++++++-- > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > index 7f6a0ae5028b..ba6845225b65 100644 > > > --- a/kernel/bpf/btf.c > > > +++ b/kernel/bpf/btf.c > > > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > return -EINVAL; > > > } > > > > > > + if (is_kfunc) > > > + rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), > > > + BTF_KFUNC_TYPE_RELEASE, func_id); > > > /* check that BTF function arguments match actual types that the > > > * verifier sees. > > > */ > > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > regno, reg->ref_obj_id, ref_obj_id); > > > return -EFAULT; > > > } > > > + /* Ensure that offset of referenced PTR_TO_BTF_ID is > > > + * always zero, when passed to release function. > > > + * var_off has already been checked to be 0 by > > > + * check_func_arg_reg_off. > > > + */ > > > + if (rel && reg->off) { > > Here is another reg->off check for PTR_TO_BTF_ID on top of the > > one 'check_func_arg_reg_off' added to the same function in patch 2. > > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted > > considering the btf func does not need ARG_* to begin with. > > > > Right, arg_type doesn't really matter here (unless we start indicating in BTF we > want to take ringbuf allocation directly without size parameter or getting size > from BTF type). > > > How about directly use the __check_ptr_off_reg() here instead of > > check_func_arg_reg_off()? Then patch 1 is not needed. > > > > Would something like this do the same thing (uncompiled code) ? > > > > I should have included a link to the previous discussion, sorry about that: > https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion Ah. Thanks for the link. I didn't go back to the list since the set is tagged v1 ;) > Yes, this should also do the same thing, but the idea was to avoid keeping the > same checks in multiple places. For now, there is only the special case of > ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling, > the former of which is currently not relevant for kfunc, but adding some future > type and ensuring kfunc, and helper do the offset checks correctly just means > updating check_func_arg_reg_off. > > reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special > case. We should also do the same thing for BPF helpers, now that I look at it, > but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and > it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it > is quite possible to support it in more BPF helpers later and forget to prevent > such case. > > So, it would be possible to move this check inside check_func_arg_reg_off, based > on a new bool is_release_func parameter, and relying on the assumption that only > one referenced register can be passed to helper or kfunc at a time (already > enforced for both BPF helpers and kfuncs). > > Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we > will do: > > fixed_off_ok = false; > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > fixed_off_ok = true; For the preemptive fix on release func and non zero reg->off, should it be a release-without-acquire error instead of a ptr-type/reg->off error? The fix should be either clearing the reg->ref_obj_id earlier or at least treat ref_obj_id as zero here and then fallthrough the existing release-without-acquire error. It is more to do with the ref_obj_id becomes invalid after reg->off becoming non-zero instead of reg->off is not allowed for a specific ptr type. It is better to separate this preemptive fix to another set. > > Again, given we can only pass one referenced reg, if we see release func and a > reg with ref_obj_id, it is the one being released. > > In the end, it's more of a preference thing, if you feel strongly about it I can > go with the __check_ptr_off_reg call too. Yeah, it is a preference thing and not feeling strongly. Without the need for the release-func/reg->off preemptive fix, adding one __check_ptr_off_reg() seems to be a cleaner fix to me but I won't insist.
On Thu, Mar 03, 2022 at 03:26:40AM IST, Martin KaFai Lau wrote: > On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote: > > On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote: > > > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc > > > > always has its offset set to 0. While not a real problem now, there's a > > > > very real possibility this will become a problem when more and more > > > > kfuncs are exposed. > > > > > > > > Previous commits already protected against non-zero var_off. The case we > > > > are concerned about now is when we have a type that can be returned by > > > > acquire kfunc: > > > > > > > > struct foo { > > > > int a; > > > > int b; > > > > struct bar b; > > > > }; > > > > > > > > ... and struct bar is also a type that can be returned by another > > > > acquire kfunc. > > > > > > > > Then, doing the following sequence: > > > > > > > > struct foo *f = bpf_get_foo(); // acquire kfunc > > > > if (!f) > > > > return 0; > > > > bpf_put_bar(&f->b); // release kfunc > > > > > > > > ... would work with the current code, since the btf_struct_ids_match > > > > takes reg->off into account for matching pointer type with release kfunc > > > > argument type, but would obviously be incorrect, and most likely lead to > > > > a kernel crash. A test has been included later to prevent regressions in > > > > this area. > > > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > --- > > > > kernel/bpf/btf.c | 15 +++++++++++++-- > > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > > index 7f6a0ae5028b..ba6845225b65 100644 > > > > --- a/kernel/bpf/btf.c > > > > +++ b/kernel/bpf/btf.c > > > > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > return -EINVAL; > > > > } > > > > > > > > + if (is_kfunc) > > > > + rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), > > > > + BTF_KFUNC_TYPE_RELEASE, func_id); > > > > /* check that BTF function arguments match actual types that the > > > > * verifier sees. > > > > */ > > > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > regno, reg->ref_obj_id, ref_obj_id); > > > > return -EFAULT; > > > > } > > > > + /* Ensure that offset of referenced PTR_TO_BTF_ID is > > > > + * always zero, when passed to release function. > > > > + * var_off has already been checked to be 0 by > > > > + * check_func_arg_reg_off. > > > > + */ > > > > + if (rel && reg->off) { > > > Here is another reg->off check for PTR_TO_BTF_ID on top of the > > > one 'check_func_arg_reg_off' added to the same function in patch 2. > > > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted > > > considering the btf func does not need ARG_* to begin with. > > > > > > > Right, arg_type doesn't really matter here (unless we start indicating in BTF we > > want to take ringbuf allocation directly without size parameter or getting size > > from BTF type). > > > > > How about directly use the __check_ptr_off_reg() here instead of > > > check_func_arg_reg_off()? Then patch 1 is not needed. > > > > > > Would something like this do the same thing (uncompiled code) ? > > > > > > > I should have included a link to the previous discussion, sorry about that: > > https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion > Ah. Thanks for the link. I didn't go back to the list since the set is > tagged v1 ;) > Right, I split the first patch out and then added this patch, so it felt more appropriate to tag it as v1. But I will include this link in the cover letter going forward. > > Yes, this should also do the same thing, but the idea was to avoid keeping the > > same checks in multiple places. For now, there is only the special case of > > ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling, > > the former of which is currently not relevant for kfunc, but adding some future > > type and ensuring kfunc, and helper do the offset checks correctly just means > > updating check_func_arg_reg_off. > > > > reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special > > case. We should also do the same thing for BPF helpers, now that I look at it, > > but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and > > it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it > > is quite possible to support it in more BPF helpers later and forget to prevent > > such case. > > > > So, it would be possible to move this check inside check_func_arg_reg_off, based > > on a new bool is_release_func parameter, and relying on the assumption that only > > one referenced register can be passed to helper or kfunc at a time (already > > enforced for both BPF helpers and kfuncs). > > > > Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we > > will do: > > > > fixed_off_ok = false; > > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > > fixed_off_ok = true; > For the preemptive fix on release func and non zero reg->off, > should it be a release-without-acquire error instead of a ptr-type/reg->off error? > The fix should be either clearing the reg->ref_obj_id earlier or at least treat > ref_obj_id as zero here and then fallthrough the existing release-without-acquire > error. It is more to do with the ref_obj_id becomes invalid after reg->off > becoming non-zero instead of reg->off is not allowed for a specific ptr > type. It is better to separate this preemptive fix to another set. > So IIUC what you're saying is that once someone performs increment, we reset the ref_obj_id to 0, then the reference state is still present so check_reference_leak would complain, but releasing such modified register won't work since ref_obj_id is 0 (so no ref state for that ref_obj_id). But I think clang (or even user writing BPF ASM) would be well within its rights to temporarily add an offset to the register, pass member pointer to some other helper, or read some data, and then decrement it again to shift the pointer backwards setting reg->off to 0. Then they should be able to again pass such register to release helper or kfunc. I think it would be unlikely (you can save original pointer to caller saved reg, or spill to stack, or use offset in LDX, etc.) but certainly not impossible. I think the key point is that we want to make user pass the register as it was when it was acquired, they can do any changes to off between acquire and release, just that it should be set back to 0 when release function is called. > > > > Again, given we can only pass one referenced reg, if we see release func and a > > reg with ref_obj_id, it is the one being released. > > > > In the end, it's more of a preference thing, if you feel strongly about it I can > > go with the __check_ptr_off_reg call too. > Yeah, it is a preference thing and not feeling strongly. > Without the need for the release-func/reg->off preemptive fix, adding > one __check_ptr_off_reg() seems to be a cleaner fix to me but > I won't insist. -- Kartikeya
On Thu, Mar 03, 2022 at 04:00:20AM +0530, Kumar Kartikeya Dwivedi wrote: > On Thu, Mar 03, 2022 at 03:26:40AM IST, Martin KaFai Lau wrote: > > On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote: > > > On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote: > > > > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc > > > > > always has its offset set to 0. While not a real problem now, there's a > > > > > very real possibility this will become a problem when more and more > > > > > kfuncs are exposed. > > > > > > > > > > Previous commits already protected against non-zero var_off. The case we > > > > > are concerned about now is when we have a type that can be returned by > > > > > acquire kfunc: > > > > > > > > > > struct foo { > > > > > int a; > > > > > int b; > > > > > struct bar b; > > > > > }; > > > > > > > > > > ... and struct bar is also a type that can be returned by another > > > > > acquire kfunc. > > > > > > > > > > Then, doing the following sequence: > > > > > > > > > > struct foo *f = bpf_get_foo(); // acquire kfunc > > > > > if (!f) > > > > > return 0; > > > > > bpf_put_bar(&f->b); // release kfunc > > > > > > > > > > ... would work with the current code, since the btf_struct_ids_match > > > > > takes reg->off into account for matching pointer type with release kfunc > > > > > argument type, but would obviously be incorrect, and most likely lead to > > > > > a kernel crash. A test has been included later to prevent regressions in > > > > > this area. > > > > > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > --- > > > > > kernel/bpf/btf.c | 15 +++++++++++++-- > > > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > > > index 7f6a0ae5028b..ba6845225b65 100644 > > > > > --- a/kernel/bpf/btf.c > > > > > +++ b/kernel/bpf/btf.c > > > > > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > > return -EINVAL; > > > > > } > > > > > > > > > > + if (is_kfunc) > > > > > + rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), > > > > > + BTF_KFUNC_TYPE_RELEASE, func_id); > > > > > /* check that BTF function arguments match actual types that the > > > > > * verifier sees. > > > > > */ > > > > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > > regno, reg->ref_obj_id, ref_obj_id); > > > > > return -EFAULT; > > > > > } > > > > > + /* Ensure that offset of referenced PTR_TO_BTF_ID is > > > > > + * always zero, when passed to release function. > > > > > + * var_off has already been checked to be 0 by > > > > > + * check_func_arg_reg_off. > > > > > + */ > > > > > + if (rel && reg->off) { > > > > Here is another reg->off check for PTR_TO_BTF_ID on top of the > > > > one 'check_func_arg_reg_off' added to the same function in patch 2. > > > > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted > > > > considering the btf func does not need ARG_* to begin with. > > > > > > > > > > Right, arg_type doesn't really matter here (unless we start indicating in BTF we > > > want to take ringbuf allocation directly without size parameter or getting size > > > from BTF type). > > > > > > > How about directly use the __check_ptr_off_reg() here instead of > > > > check_func_arg_reg_off()? Then patch 1 is not needed. > > > > > > > > Would something like this do the same thing (uncompiled code) ? > > > > > > > > > > I should have included a link to the previous discussion, sorry about that: > > > https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion > > Ah. Thanks for the link. I didn't go back to the list since the set is > > tagged v1 ;) > > > > Right, I split the first patch out and then added this patch, so it felt more > appropriate to tag it as v1. But I will include this link in the cover letter > going forward. > > > > Yes, this should also do the same thing, but the idea was to avoid keeping the > > > same checks in multiple places. For now, there is only the special case of > > > ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling, > > > the former of which is currently not relevant for kfunc, but adding some future > > > type and ensuring kfunc, and helper do the offset checks correctly just means > > > updating check_func_arg_reg_off. > > > > > > reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special > > > case. We should also do the same thing for BPF helpers, now that I look at it, > > > but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and > > > it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it > > > is quite possible to support it in more BPF helpers later and forget to prevent > > > such case. > > > > > > So, it would be possible to move this check inside check_func_arg_reg_off, based > > > on a new bool is_release_func parameter, and relying on the assumption that only > > > one referenced register can be passed to helper or kfunc at a time (already > > > enforced for both BPF helpers and kfuncs). > > > > > > Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we > > > will do: > > > > > > fixed_off_ok = false; > > > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > > > fixed_off_ok = true; > > For the preemptive fix on release func and non zero reg->off, > > should it be a release-without-acquire error instead of a ptr-type/reg->off error? > > The fix should be either clearing the reg->ref_obj_id earlier or at least treat > > ref_obj_id as zero here and then fallthrough the existing release-without-acquire > > error. It is more to do with the ref_obj_id becomes invalid after reg->off > > becoming non-zero instead of reg->off is not allowed for a specific ptr > > type. It is better to separate this preemptive fix to another set. > > > > So IIUC what you're saying is that once someone performs increment, we reset the > ref_obj_id to 0, then the reference state is still present so > check_reference_leak would complain, but releasing such modified register won't > work since ref_obj_id is 0 (so no ref state for that ref_obj_id). > > But I think clang (or even user writing BPF ASM) would be well within its rights > to temporarily add an offset to the register, pass member pointer to some other > helper, or read some data, and then decrement it again to shift the pointer > backwards setting reg->off to 0. Then they should be able to again pass such > register to release helper or kfunc. I think it would be unlikely (you can save > original pointer to caller saved reg, or spill to stack, or use offset in LDX, > etc.) but certainly not impossible. I don't think llvm will ever do such thing. Passing into a helper means that the register is scratched. It won't be reused after the call. Saving modified into a stack to restore later just to do a math on it goes against "optimization" goal of the compiler. > I think the key point is that we want to make user pass the register as it was > when it was acquired, they can do any changes to off between acquire and > release, just that it should be set back to 0 when release function is called. Correct and this patch is covering that. I'm not sure what is the contention point here. Sorry I'm behind the mailing list. > > > > > > Again, given we can only pass one referenced reg, if we see release func and a > > > reg with ref_obj_id, it is the one being released. > > > > > > In the end, it's more of a preference thing, if you feel strongly about it I can > > > go with the __check_ptr_off_reg call too. > > Yeah, it is a preference thing and not feeling strongly. > > Without the need for the release-func/reg->off preemptive fix, adding > > one __check_ptr_off_reg() seems to be a cleaner fix to me but > > I won't insist. fwiw I like patches 1-3. I think extra check here for release func is justified on its own. Converting it into: fixed_off_ok = false; if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) fixed_off_ok = true; obfuscates the check to me. if (rel && reg->off) check is pretty obvious.
On Thu, Mar 03, 2022 at 04:14:18AM IST, Alexei Starovoitov wrote: > On Thu, Mar 03, 2022 at 04:00:20AM +0530, Kumar Kartikeya Dwivedi wrote: > > On Thu, Mar 03, 2022 at 03:26:40AM IST, Martin KaFai Lau wrote: > > > On Wed, Mar 02, 2022 at 03:12:18PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > On Wed, Mar 02, 2022 at 08:50:24AM IST, Martin KaFai Lau wrote: > > > > > On Tue, Mar 01, 2022 at 12:27:43PM +0530, Kumar Kartikeya Dwivedi wrote: > > > > > > Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc > > > > > > always has its offset set to 0. While not a real problem now, there's a > > > > > > very real possibility this will become a problem when more and more > > > > > > kfuncs are exposed. > > > > > > > > > > > > Previous commits already protected against non-zero var_off. The case we > > > > > > are concerned about now is when we have a type that can be returned by > > > > > > acquire kfunc: > > > > > > > > > > > > struct foo { > > > > > > int a; > > > > > > int b; > > > > > > struct bar b; > > > > > > }; > > > > > > > > > > > > ... and struct bar is also a type that can be returned by another > > > > > > acquire kfunc. > > > > > > > > > > > > Then, doing the following sequence: > > > > > > > > > > > > struct foo *f = bpf_get_foo(); // acquire kfunc > > > > > > if (!f) > > > > > > return 0; > > > > > > bpf_put_bar(&f->b); // release kfunc > > > > > > > > > > > > ... would work with the current code, since the btf_struct_ids_match > > > > > > takes reg->off into account for matching pointer type with release kfunc > > > > > > argument type, but would obviously be incorrect, and most likely lead to > > > > > > a kernel crash. A test has been included later to prevent regressions in > > > > > > this area. > > > > > > > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > > --- > > > > > > kernel/bpf/btf.c | 15 +++++++++++++-- > > > > > > 1 file changed, 13 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > > > > index 7f6a0ae5028b..ba6845225b65 100644 > > > > > > --- a/kernel/bpf/btf.c > > > > > > +++ b/kernel/bpf/btf.c > > > > > > @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > > > + if (is_kfunc) > > > > > > + rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), > > > > > > + BTF_KFUNC_TYPE_RELEASE, func_id); > > > > > > /* check that BTF function arguments match actual types that the > > > > > > * verifier sees. > > > > > > */ > > > > > > @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > > > > > > regno, reg->ref_obj_id, ref_obj_id); > > > > > > return -EFAULT; > > > > > > } > > > > > > + /* Ensure that offset of referenced PTR_TO_BTF_ID is > > > > > > + * always zero, when passed to release function. > > > > > > + * var_off has already been checked to be 0 by > > > > > > + * check_func_arg_reg_off. > > > > > > + */ > > > > > > + if (rel && reg->off) { > > > > > Here is another reg->off check for PTR_TO_BTF_ID on top of the > > > > > one 'check_func_arg_reg_off' added to the same function in patch 2. > > > > > A nit, I also found passing ARG_DONTCARE in patch 2 a bit convoluted > > > > > considering the btf func does not need ARG_* to begin with. > > > > > > > > > > > > > Right, arg_type doesn't really matter here (unless we start indicating in BTF we > > > > want to take ringbuf allocation directly without size parameter or getting size > > > > from BTF type). > > > > > > > > > How about directly use the __check_ptr_off_reg() here instead of > > > > > check_func_arg_reg_off()? Then patch 1 is not needed. > > > > > > > > > > Would something like this do the same thing (uncompiled code) ? > > > > > > > > > > > > > I should have included a link to the previous discussion, sorry about that: > > > > https://lore.kernel.org/bpf/20220223031600.pvbhu3dbwxke4eia@apollo.legion > > > Ah. Thanks for the link. I didn't go back to the list since the set is > > > tagged v1 ;) > > > > > > > Right, I split the first patch out and then added this patch, so it felt more > > appropriate to tag it as v1. But I will include this link in the cover letter > > going forward. > > > > > > Yes, this should also do the same thing, but the idea was to avoid keeping the > > > > same checks in multiple places. For now, there is only the special case of > > > > ARG_TYPE_PTR_TO_ALLOC_MEM and PTR_TO_BTF_ID that require some special handling, > > > > the former of which is currently not relevant for kfunc, but adding some future > > > > type and ensuring kfunc, and helper do the offset checks correctly just means > > > > updating check_func_arg_reg_off. > > > > > > > > reg->off in case of PTR_TO_BTF_ID reg for release kfunc is a bit of a special > > > > case. We should also do the same thing for BPF helpers, now that I look at it, > > > > but there's only one which takes a PTR_TO_BTF_ID right now (bpf_sk_release), and > > > > it isn't problematic currently, but now that referenced PTR_TO_BTF_ID is used it > > > > is quite possible to support it in more BPF helpers later and forget to prevent > > > > such case. > > > > > > > > So, it would be possible to move this check inside check_func_arg_reg_off, based > > > > on a new bool is_release_func parameter, and relying on the assumption that only > > > > one referenced register can be passed to helper or kfunc at a time (already > > > > enforced for both BPF helpers and kfuncs). > > > > > > > > Basically, instead of doing type == PTR_TO_BTF_ID for fixed_off_ok inside it, we > > > > will do: > > > > > > > > fixed_off_ok = false; > > > > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > > > > fixed_off_ok = true; > > > For the preemptive fix on release func and non zero reg->off, > > > should it be a release-without-acquire error instead of a ptr-type/reg->off error? > > > The fix should be either clearing the reg->ref_obj_id earlier or at least treat > > > ref_obj_id as zero here and then fallthrough the existing release-without-acquire > > > error. It is more to do with the ref_obj_id becomes invalid after reg->off > > > becoming non-zero instead of reg->off is not allowed for a specific ptr > > > type. It is better to separate this preemptive fix to another set. > > > > > > > So IIUC what you're saying is that once someone performs increment, we reset the > > ref_obj_id to 0, then the reference state is still present so > > check_reference_leak would complain, but releasing such modified register won't > > work since ref_obj_id is 0 (so no ref state for that ref_obj_id). > > > > But I think clang (or even user writing BPF ASM) would be well within its rights > > to temporarily add an offset to the register, pass member pointer to some other > > helper, or read some data, and then decrement it again to shift the pointer > > backwards setting reg->off to 0. Then they should be able to again pass such > > register to release helper or kfunc. I think it would be unlikely (you can save > > original pointer to caller saved reg, or spill to stack, or use offset in LDX, > > etc.) but certainly not impossible. > > I don't think llvm will ever do such thing. Passing into a helper means > that the register is scratched. It won't be reused after the call. > Saving modified into a stack to restore later just to do a math on it > goes against "optimization" goal of the compiler. > > > I think the key point is that we want to make user pass the register as it was > > when it was acquired, they can do any changes to off between acquire and > > release, just that it should be set back to 0 when release function is called. > > Correct and this patch is covering that. > I'm not sure what is the contention point here. > Sorry I'm behind the mailing list. > > > > > > > > > Again, given we can only pass one referenced reg, if we see release func and a > > > > reg with ref_obj_id, it is the one being released. > > > > > > > > In the end, it's more of a preference thing, if you feel strongly about it I can > > > > go with the __check_ptr_off_reg call too. > > > Yeah, it is a preference thing and not feeling strongly. > > > Without the need for the release-func/reg->off preemptive fix, adding > > > one __check_ptr_off_reg() seems to be a cleaner fix to me but > > > I won't insist. > > fwiw I like patches 1-3. > I think extra check here for release func is justified on its own. > Converting it into: > fixed_off_ok = false; > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > fixed_off_ok = true; > obfuscates the check to me. I was talking of putting this inside check_func_arg_reg_off. I think we should do the same check for BPF helpers as well (rn only one supports releasing PTR_TO_BTF_ID, soon we may have others). Just passing a bool to check_func_arg_reg_off to indicate we are checking for release func (helper or kfunc have same rules here) would allow putting this check inside it. > if (rel && reg->off) check > is pretty obvious. -- Kartikeya
On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > fwiw I like patches 1-3. > > I think extra check here for release func is justified on its own. > > Converting it into: > > fixed_off_ok = false; > > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > > fixed_off_ok = true; > > obfuscates the check to me. > > I was talking of putting this inside check_func_arg_reg_off. I think we should > do the same check for BPF helpers as well (rn only one supports releasing > PTR_TO_BTF_ID, soon we may have others). Just passing a bool to > check_func_arg_reg_off to indicate we are checking for release func (helper or > kfunc have same rules here) would allow putting this check inside it. Hmm. check_func_arg() is called before we call is_release_function(func_id). Are you proposing to call it before and pass another boolean into check_func_arg() or store the flag in meta? Sounds ugly. imo reg->off is a simple enough check to keep it open coded where necessary.
On Thu, Mar 03, 2022 at 04:47:59AM IST, Alexei Starovoitov wrote: > On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > fwiw I like patches 1-3. > > > I think extra check here for release func is justified on its own. > > > Converting it into: > > > fixed_off_ok = false; > > > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > > > fixed_off_ok = true; > > > obfuscates the check to me. > > > > I was talking of putting this inside check_func_arg_reg_off. I think we should > > do the same check for BPF helpers as well (rn only one supports releasing > > PTR_TO_BTF_ID, soon we may have others). Just passing a bool to > > check_func_arg_reg_off to indicate we are checking for release func (helper or > > kfunc have same rules here) would allow putting this check inside it. > > Hmm. check_func_arg() is called before we call > is_release_function(func_id). > Are you proposing to call it before and pass > another boolean into check_func_arg() or store the flag in meta? We save meta.func_id before calling check_func_arg. Inside it we can do: err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id)); I actually tried open coding it for BPF helpers, and it was more complicated. If we delay this check until is_release_function call after check_func_arg, we need to remember if reg for whom meta->ref_obj_id had off > 0 and type PTR_TO_BTF_ID. If we put it inside check_reg_type or check_func_arg, you need to call is_release_function anyway there. Compared to these two options, doing it in check_func_arg_reg_off looks better to me, but ymmv. > Sounds ugly. > imo reg->off is a simple enough check to keep it open coded > where necessary. -- Kartikeya
On Wed, Mar 02, 2022 at 02:44:18PM -0800, Alexei Starovoitov wrote: > > So IIUC what you're saying is that once someone performs increment, we reset the > > ref_obj_id to 0, then the reference state is still present so > > check_reference_leak would complain, but releasing such modified register won't > > work since ref_obj_id is 0 (so no ref state for that ref_obj_id). I meant error similar to release_reference() should be used and it should be treated as release-without-prior-acquire error. The reg is pointing at something that its reference has not been acquired before. You are right, reset ref_obj_id to 0 during increment won't work as you have explained in the following. > > > > But I think clang (or even user writing BPF ASM) would be well within its rights > > to temporarily add an offset to the register, pass member pointer to some other > > helper, or read some data, and then decrement it again to shift the pointer > > backwards setting reg->off to 0. Then they should be able to again pass such > > register to release helper or kfunc. I think it would be unlikely (you can save > > original pointer to caller saved reg, or spill to stack, or use offset in LDX, > > etc.) but certainly not impossible. > > I don't think llvm will ever do such thing. Passing into a helper means > that the register is scratched. It won't be reused after the call. > Saving modified into a stack to restore later just to do a math on it > goes against "optimization" goal of the compiler. > > > I think the key point is that we want to make user pass the register as it was > > when it was acquired, they can do any changes to off between acquire and > > release, just that it should be set back to 0 when release function is called. > > Correct and this patch is covering that. > I'm not sure what is the contention point here. > Sorry I'm behind the mailing list. > > > > > > > > > Again, given we can only pass one referenced reg, if we see release func and a > > > > reg with ref_obj_id, it is the one being released. > > > > > > > > In the end, it's more of a preference thing, if you feel strongly about it I can > > > > go with the __check_ptr_off_reg call too. > > > Yeah, it is a preference thing and not feeling strongly. > > > Without the need for the release-func/reg->off preemptive fix, adding > > > one __check_ptr_off_reg() seems to be a cleaner fix to me but > > > I won't insist. > > fwiw I like patches 1-3. > I think extra check here for release func is justified on its own. > Converting it into: > fixed_off_ok = false; > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > fixed_off_ok = true; > obfuscates the check to me. > if (rel && reg->off) check > is pretty obvious. Yeah, I am fine with an extra check and the "must have zero offset message when passed to release kfunc". The error is obvious enough on what may be wrong in the bpf prog.
On Wed, Mar 2, 2022 at 3:29 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, Mar 03, 2022 at 04:47:59AM IST, Alexei Starovoitov wrote: > > On Wed, Mar 2, 2022 at 3:00 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > > > > > > > fwiw I like patches 1-3. > > > > I think extra check here for release func is justified on its own. > > > > Converting it into: > > > > fixed_off_ok = false; > > > > if (type == PTR_TO_BTF_ID && (!is_release_func || !reg->ref_obj_id)) > > > > fixed_off_ok = true; > > > > obfuscates the check to me. > > > > > > I was talking of putting this inside check_func_arg_reg_off. I think we should > > > do the same check for BPF helpers as well (rn only one supports releasing > > > PTR_TO_BTF_ID, soon we may have others). Just passing a bool to > > > check_func_arg_reg_off to indicate we are checking for release func (helper or > > > kfunc have same rules here) would allow putting this check inside it. > > > > Hmm. check_func_arg() is called before we call > > is_release_function(func_id). > > Are you proposing to call it before and pass > > another boolean into check_func_arg() or store the flag in meta? > > We save meta.func_id before calling check_func_arg. Inside it we can do: > err = check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id)); > > I actually tried open coding it for BPF helpers, and it was more complicated. If > we delay this check until is_release_function call after check_func_arg, we need > to remember if reg for whom meta->ref_obj_id had off > 0 and type PTR_TO_BTF_ID. > If we put it inside check_reg_type or check_func_arg, you need to call > is_release_function anyway there. > > Compared to these two options, doing it in check_func_arg_reg_off looks better > to me, but ymmv. I see. Yeah. You're right. check_func_arg_reg_off(env, reg, regno, arg_type, is_release_function(meta->func_id)) is indeed cleaner.
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7f6a0ae5028b..ba6845225b65 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -5753,6 +5753,9 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, return -EINVAL; } + if (is_kfunc) + rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), + BTF_KFUNC_TYPE_RELEASE, func_id); /* check that BTF function arguments match actual types that the * verifier sees. */ @@ -5816,6 +5819,16 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, regno, reg->ref_obj_id, ref_obj_id); return -EFAULT; } + /* Ensure that offset of referenced PTR_TO_BTF_ID is + * always zero, when passed to release function. + * var_off has already been checked to be 0 by + * check_func_arg_reg_off. + */ + if (rel && reg->off) { + bpf_log(log, "R%d with ref_obj_id=%d must have zero offset when passed to release kfunc\n", + regno, reg->ref_obj_id); + return -EINVAL; + } ref_regno = regno; ref_obj_id = reg->ref_obj_id; } @@ -5892,8 +5905,6 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, /* Either both are set, or neither */ WARN_ON_ONCE((ref_obj_id && !ref_regno) || (!ref_obj_id && ref_regno)); if (is_kfunc) { - rel = btf_kfunc_id_set_contains(btf, resolve_prog_type(env->prog), - BTF_KFUNC_TYPE_RELEASE, func_id); /* We already made sure ref_obj_id is set only for one argument */ if (rel && !ref_obj_id) { bpf_log(log, "release kernel function %s expects refcounted PTR_TO_BTF_ID\n",
Let's ensure that the PTR_TO_BTF_ID reg being passed in to release kfunc always has its offset set to 0. While not a real problem now, there's a very real possibility this will become a problem when more and more kfuncs are exposed. Previous commits already protected against non-zero var_off. The case we are concerned about now is when we have a type that can be returned by acquire kfunc: struct foo { int a; int b; struct bar b; }; ... and struct bar is also a type that can be returned by another acquire kfunc. Then, doing the following sequence: struct foo *f = bpf_get_foo(); // acquire kfunc if (!f) return 0; bpf_put_bar(&f->b); // release kfunc ... would work with the current code, since the btf_struct_ids_match takes reg->off into account for matching pointer type with release kfunc argument type, but would obviously be incorrect, and most likely lead to a kernel crash. A test has been included later to prevent regressions in this area. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/btf.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)