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

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

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,
>  							    &reg_ref_id);
>  			reg_ref_tname = btf_name_by_offset(reg_btf,
>

--
Kartikeya