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@xxxxxxxxx>
> > > ---
> > > 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.
