| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 7 Sep 2022 19:18:58 +0200
From: Kumar Kartikeya Dwivedi <memxor@...il.com>
To: Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc: Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
bpf@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next v11 2/7] bpf: split btf_check_subprog_arg_match
in two
On Tue, 6 Sept 2022 at 17:13, Benjamin Tissoires
<benjamin.tissoires@...hat.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@...hat.com>
>
> ---
>
Given I'll fix it properly in my kfunc rework, LGTM otherwise:
Acked-by: Kumar Kartikeya Dwivedi <memxor@...il.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
>
Powered by blists - more mailing lists