Re: [PATCH bpf-next v1 4/6] bpf: Harden register offset checks for release kfunc

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

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