| Message ID | 20220906151303.2780789-3-benjamin.tissoires@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 95f2f26f3cac06cfc046d2b29e60719d7848ea54 |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf-core changes for preparation of | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for bpf-next, async |
| netdev/apply | fail | Patch does not apply to bpf-next |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-4 | success | Logs for llvm-toolchain |
| bpf/vmtest-bpf-next-VM_Test-5 | success | Logs for set-matrix |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for build for x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for build for x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for build for s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-16 | success | Logs for test_verifier on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-17 | success | Logs for test_verifier on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-10 | success | Logs for test_progs on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-11 | success | Logs for test_progs on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-13 | fail | Logs for test_progs_no_alu32 on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-14 | fail | Logs for test_progs_no_alu32 on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-7 | success | Logs for test_maps on x86_64 with gcc |
| bpf/vmtest-bpf-next-VM_Test-8 | success | Logs for test_maps on x86_64 with llvm-16 |
| bpf/vmtest-bpf-next-VM_Test-9 | success | Logs for test_progs on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-12 | success | Logs for test_progs_no_alu32 on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-15 | success | Logs for test_verifier on s390x with gcc |
| bpf/vmtest-bpf-next-VM_Test-6 | success | Logs for test_maps on s390x with gcc |
On Tue, 6 Sept 2022 at 17:13, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > btf_check_subprog_arg_match() was used twice in verifier.c: > - when checking for the type mismatches between a (sub)prog declaration > and BTF > - when checking the call of a subprog to see if the provided arguments > are correct and valid > > This is problematic when we check if the first argument of a program > (pointer to ctx) is correctly accessed: > To be able to ensure we access a valid memory in the ctx, the verifier > assumes the pointer to context is not null. > This has the side effect of marking the program accessing the entire > context, even if the context is never dereferenced. > > For example, by checking the context access with the current code, the > following eBPF program would fail with -EINVAL if the ctx is set to null > from the userspace: > > ``` > SEC("syscall") > int prog(struct my_ctx *args) { > return 0; > } > ``` > > In that particular case, we do not want to actually check that the memory > is correct while checking for the BTF validity, but we just want to > ensure that the (sub)prog definition matches the BTF we have. > > So split btf_check_subprog_arg_match() in two so we can actually check > for the memory used when in a call, and ignore that part when not. > > Note that a further patch is in preparation to disentangled > btf_check_func_arg_match() from these two purposes, and so right now we > just add a new hack around that by adding a boolean to this function. > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > Given I'll fix it properly in my kfunc rework, LGTM otherwise: Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > no changes in v11 > > new in v10 > --- > include/linux/bpf.h | 2 ++ > kernel/bpf/btf.c | 54 +++++++++++++++++++++++++++++++++++++++---- > kernel/bpf/verifier.c | 2 +- > 3 files changed, 52 insertions(+), 6 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 9c1674973e03..c9c72a089579 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1943,6 +1943,8 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, > struct bpf_reg_state; > int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > struct bpf_reg_state *regs); > +int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog, > + struct bpf_reg_state *regs); > int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > struct bpf_reg_state *regs, > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 903719b89238..eca9ea78ee5f 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6170,7 +6170,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > const struct btf *btf, u32 func_id, > struct bpf_reg_state *regs, > bool ptr_to_mem_ok, > - u32 kfunc_flags) > + u32 kfunc_flags, > + bool processing_call) > { > enum bpf_prog_type prog_type = resolve_prog_type(env->prog); > bool rel = false, kptr_get = false, trusted_arg = false; > @@ -6356,7 +6357,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > reg_ref_tname); > return -EINVAL; > } > - } else if (ptr_to_mem_ok) { > + } else if (ptr_to_mem_ok && processing_call) { > const struct btf_type *resolve_ret; > u32 type_size; > > @@ -6431,7 +6432,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, > return rel ? ref_regno : 0; > } > > -/* Compare BTF of a function with given bpf_reg_state. > +/* Compare BTF of a function declaration with given bpf_reg_state. > * Returns: > * EFAULT - there is a verifier bug. Abort verification. > * EINVAL - there is a type mismatch or BTF is not available. > @@ -6458,7 +6459,50 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, > return -EINVAL; > > is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0); > + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, false); > + > + /* Compiler optimizations can remove arguments from static functions > + * or mismatched type can be passed into a global function. > + * In such cases mark the function as unreliable from BTF point of view. > + */ > + if (err) > + prog->aux->func_info_aux[subprog].unreliable = true; > + return err; > +} > + > +/* Compare BTF of a function call with given bpf_reg_state. > + * Returns: > + * EFAULT - there is a verifier bug. Abort verification. > + * EINVAL - there is a type mismatch or BTF is not available. > + * 0 - BTF matches with what bpf_reg_state expects. > + * Only PTR_TO_CTX and SCALAR_VALUE states are recognized. > + * > + * NOTE: the code is duplicated from btf_check_subprog_arg_match() > + * because btf_check_func_arg_match() is still doing both. Once that > + * function is split in 2, we can call from here btf_check_subprog_arg_match() > + * first, and then treat the calling part in a new code path. > + */ > +int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog, > + struct bpf_reg_state *regs) > +{ > + struct bpf_prog *prog = env->prog; > + struct btf *btf = prog->aux->btf; > + bool is_global; > + u32 btf_id; > + int err; > + > + if (!prog->aux->func_info) > + return -EINVAL; > + > + btf_id = prog->aux->func_info[subprog].type_id; > + if (!btf_id) > + return -EFAULT; > + > + if (prog->aux->func_info_aux[subprog].unreliable) > + return -EINVAL; > + > + is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, true); > > /* Compiler optimizations can remove arguments from static functions > * or mismatched type can be passed into a global function. > @@ -6474,7 +6518,7 @@ int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, > struct bpf_reg_state *regs, > u32 kfunc_flags) > { > - return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags); > + return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags, true); > } > > /* Convert BTF of a function into bpf_reg_state if possible > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 0194a36d0b36..d27fae3ce949 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -6626,7 +6626,7 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn > func_info_aux = env->prog->aux->func_info_aux; > if (func_info_aux) > is_global = func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; > - err = btf_check_subprog_arg_match(env, subprog, caller->regs); > + err = btf_check_subprog_call(env, subprog, caller->regs); > if (err == -EFAULT) > return err; > if (is_global) { > -- > 2.36.1 >
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 9c1674973e03..c9c72a089579 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1943,6 +1943,8 @@ int btf_distill_func_proto(struct bpf_verifier_log *log, struct bpf_reg_state; int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, struct bpf_reg_state *regs); +int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog, + struct bpf_reg_state *regs); int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 903719b89238..eca9ea78ee5f 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6170,7 +6170,8 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id, struct bpf_reg_state *regs, bool ptr_to_mem_ok, - u32 kfunc_flags) + u32 kfunc_flags, + bool processing_call) { enum bpf_prog_type prog_type = resolve_prog_type(env->prog); bool rel = false, kptr_get = false, trusted_arg = false; @@ -6356,7 +6357,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, reg_ref_tname); return -EINVAL; } - } else if (ptr_to_mem_ok) { + } else if (ptr_to_mem_ok && processing_call) { const struct btf_type *resolve_ret; u32 type_size; @@ -6431,7 +6432,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env, return rel ? ref_regno : 0; } -/* Compare BTF of a function with given bpf_reg_state. +/* Compare BTF of a function declaration with given bpf_reg_state. * Returns: * EFAULT - there is a verifier bug. Abort verification. * EINVAL - there is a type mismatch or BTF is not available. @@ -6458,7 +6459,50 @@ int btf_check_subprog_arg_match(struct bpf_verifier_env *env, int subprog, return -EINVAL; is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; - err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0); + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, false); + + /* Compiler optimizations can remove arguments from static functions + * or mismatched type can be passed into a global function. + * In such cases mark the function as unreliable from BTF point of view. + */ + if (err) + prog->aux->func_info_aux[subprog].unreliable = true; + return err; +} + +/* Compare BTF of a function call with given bpf_reg_state. + * Returns: + * EFAULT - there is a verifier bug. Abort verification. + * EINVAL - there is a type mismatch or BTF is not available. + * 0 - BTF matches with what bpf_reg_state expects. + * Only PTR_TO_CTX and SCALAR_VALUE states are recognized. + * + * NOTE: the code is duplicated from btf_check_subprog_arg_match() + * because btf_check_func_arg_match() is still doing both. Once that + * function is split in 2, we can call from here btf_check_subprog_arg_match() + * first, and then treat the calling part in a new code path. + */ +int btf_check_subprog_call(struct bpf_verifier_env *env, int subprog, + struct bpf_reg_state *regs) +{ + struct bpf_prog *prog = env->prog; + struct btf *btf = prog->aux->btf; + bool is_global; + u32 btf_id; + int err; + + if (!prog->aux->func_info) + return -EINVAL; + + btf_id = prog->aux->func_info[subprog].type_id; + if (!btf_id) + return -EFAULT; + + if (prog->aux->func_info_aux[subprog].unreliable) + return -EINVAL; + + is_global = prog->aux->func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; + err = btf_check_func_arg_match(env, btf, btf_id, regs, is_global, 0, true); /* Compiler optimizations can remove arguments from static functions * or mismatched type can be passed into a global function. @@ -6474,7 +6518,7 @@ int btf_check_kfunc_arg_match(struct bpf_verifier_env *env, struct bpf_reg_state *regs, u32 kfunc_flags) { - return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags); + return btf_check_func_arg_match(env, btf, func_id, regs, true, kfunc_flags, true); } /* Convert BTF of a function into bpf_reg_state if possible diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 0194a36d0b36..d27fae3ce949 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6626,7 +6626,7 @@ static int __check_func_call(struct bpf_verifier_env *env, struct bpf_insn *insn func_info_aux = env->prog->aux->func_info_aux; if (func_info_aux) is_global = func_info_aux[subprog].linkage == BTF_FUNC_GLOBAL; - err = btf_check_subprog_arg_match(env, subprog, caller->regs); + err = btf_check_subprog_call(env, subprog, caller->regs); if (err == -EFAULT) return err; if (is_global) {
btf_check_subprog_arg_match() was used twice in verifier.c: - when checking for the type mismatches between a (sub)prog declaration and BTF - when checking the call of a subprog to see if the provided arguments are correct and valid This is problematic when we check if the first argument of a program (pointer to ctx) is correctly accessed: To be able to ensure we access a valid memory in the ctx, the verifier assumes the pointer to context is not null. This has the side effect of marking the program accessing the entire context, even if the context is never dereferenced. For example, by checking the context access with the current code, the following eBPF program would fail with -EINVAL if the ctx is set to null from the userspace: ``` SEC("syscall") int prog(struct my_ctx *args) { return 0; } ``` In that particular case, we do not want to actually check that the memory is correct while checking for the BTF validity, but we just want to ensure that the (sub)prog definition matches the BTF we have. So split btf_check_subprog_arg_match() in two so we can actually check for the memory used when in a call, and ignore that part when not. Note that a further patch is in preparation to disentangled btf_check_func_arg_match() from these two purposes, and so right now we just add a new hack around that by adding a boolean to this function. Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- no changes in v11 new in v10 --- include/linux/bpf.h | 2 ++ kernel/bpf/btf.c | 54 +++++++++++++++++++++++++++++++++++++++---- kernel/bpf/verifier.c | 2 +- 3 files changed, 52 insertions(+), 6 deletions(-)