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.
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,
