linux-kernel - Re: [PATCH] btf: fix resolving BTF_KIND

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Mar 2023 16:16:08 -0800
From:   Martin KaFai Lau <martin.lau@...ux.dev>
To:     Lorenz Bauer <lorenz.bauer@...valent.com>
Cc:     Lorenz Bauer <lmb@...valent.com>, bpf@...r.kernel.org,
        linux-kernel@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>
Subject: Re: [PATCH] btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT,
 UNION, PTR

On 3/2/23 4:34 AM, Lorenz Bauer wrote:
> btf_datasec_resolve contains a bug that causes the following BTF
> to fail loading:
> 
>      [1] DATASEC a size=2 vlen=2
>          type_id=4 offset=0 size=1
>          type_id=7 offset=1 size=1
>      [2] INT (anon) size=1 bits_offset=0 nr_bits=8 encoding=(none)
>      [3] PTR (anon) type_id=2
>      [4] VAR a type_id=3 linkage=0
>      [5] INT (anon) size=1 bits_offset=0 nr_bits=8 encoding=(none)
>      [6] TYPEDEF td type_id=5
>      [7] VAR b type_id=6 linkage=0
> 
> This error message is printed during btf_check_all_types:
> 
>      [1] DATASEC a size=2 vlen=2
>          type_id=7 offset=1 size=1 Invalid type
> 
> By tracing btf_*_resolve we can pinpoint the problem:
> 
>      btf_datasec_resolve(depth: 1, type_id: 1, mode: RESOLVE_TBD) = 0
>          btf_var_resolve(depth: 2, type_id: 4, mode: RESOLVE_TBD) = 0
>              btf_ptr_resolve(depth: 3, type_id: 3, mode: RESOLVE_PTR) = 0
>          btf_var_resolve(depth: 2, type_id: 4, mode: RESOLVE_PTR) = 0
>      btf_datasec_resolve(depth: 1, type_id: 1, mode: RESOLVE_PTR) = -22
> 
> The last invocation of btf_datasec_resolve should invoke btf_var_resolve
> by means of env_stack_push, instead it returns EINVAL. The reason is that
> env_stack_push is never executed for the second VAR.
> 
>      if (!env_type_is_resolve_sink(env, var_type) &&
>          !env_type_is_resolved(env, var_type_id)) {
>          env_stack_set_next_member(env, i + 1);
>          return env_stack_push(env, var_type, var_type_id);
>      }
> 
> env_type_is_resolve_sink() changes its behaviour based on resolve_mode.
> For RESOLVE_PTR, we can simplify the if condition to the following:
> 
>      (btf_type_is_modifier() || btf_type_is_ptr) && !env_type_is_resolved()
> 
> Since we're dealing with a VAR the clause evaluates to false. This is
> not sufficient to trigger the bug however. The log output and EINVAL
> are only generated if btf_type_id_size() fails.
> 
>      if (!btf_type_id_size(btf, &type_id, &type_size)) {
>          btf_verifier_log_vsi(env, v->t, vsi, "Invalid type");
>          return -EINVAL;
>      }
> 
> Most types are sized, so for example a VAR referring to an INT is not a
> problem. The bug is only triggered if a VAR points at a modifier. Since
> we skipped btf_var_resolve that modifier was also never resolved, which
> means that btf_resolved_type_id returns 0 aka VOID for the modifier.
> This in turn causes btf_type_id_size to return NULL, triggering EINVAL.
> 
> To summarise, the following conditions are necessary:
> 
> - VAR pointing at PTR, STRUCT, UNION or ARRAY
> - Followed by a VAR pointing at TYPEDEF, VOLATILE, CONST, RESTRICT or
>    TYPE_TAG
> 
> The fix is to reset resolve_mode to RESOLVE_TBD before attempting to
> resolve a VAR from a DATASEC.
> 
> Fixes: 1dc92851849c ("bpf: kernel side support for BTF Var and DataSec")
> Signed-off-by: Lorenz Bauer <lmb@...valent.com>
> ---
>   kernel/bpf/btf.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index fa22ec79ac0e..91145298c238 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -4579,6 +4579,7 @@ static int btf_datasec_resolve(struct btf_verifier_env *env,
>   			return -EINVAL;
>   		}
>   
> +		env->resolve_mode = RESOLVE_TBD;

lgtm. Could it be moved out of the for loop?

Please add the test case described in the commit message to the prog_tests/btf.c.

>   		if (!env_type_is_resolve_sink(env, var_type) &&
>   		    !env_type_is_resolved(env, var_type_id)) {
>   			env_stack_set_next_member(env, i + 1);