netdev - [PATCH bpf] bpf: fix BTF verifier size resolution logic

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-next>] [day] [month] [year] [list]

Message-ID: <20190710080840.2613160-1-andriin@fb.com>
Date:   Wed, 10 Jul 2019 01:08:40 -0700
From:   Andrii Nakryiko <andriin@...com>
To:     <andrii.nakryiko@...il.com>, <ast@...com>, <daniel@...earbox.net>,
        <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
        <kernel-team@...com>
CC:     Andrii Nakryiko <andriin@...com>, Martin KaFai Lau <kafai@...com>
Subject: [PATCH bpf] bpf: fix BTF verifier size resolution logic

BTF verifier has Different logic depending on whether we are following
a PTR or STRUCT/ARRAY (or something else). This is an optimization to
stop early in DFS traversal while resolving BTF types. But it also
results in a size resolution bug, when there is a chain, e.g., of PTR ->
TYPEDEF -> ARRAY, in which case due to being in pointer context ARRAY
size won't be resolved, as it is considered to be a sink for pointer,
leading to TYPEDEF being in RESOLVED state with zero size, which is
completely wrong.

Optimization is doubtful, though, as btf_check_all_types() will iterate
over all BTF types anyways, so the only saving is a potentially slightly
shorter stack. But correctness is more important that tiny savings.

This bug manifests itself in rejecting BTF-defined maps that use array
typedef as a value type:

typedef int array_t[16];

struct {
	__uint(type, BPF_MAP_TYPE_ARRAY);
	__type(value, array_t); /* i.e., array_t *value; */
} test_map SEC(".maps");

Fixes: eb3f595dab40 ("bpf: btf: Validate type reference")
Cc: Martin KaFai Lau <kafai@...com>
Signed-off-by: Andrii Nakryiko <andriin@...com>
---
 kernel/bpf/btf.c | 42 +++---------------------------------------
 1 file changed, 3 insertions(+), 39 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index cad09858a5f2..c68c7e73b0d1 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -231,14 +231,6 @@ enum visit_state {
 	RESOLVED,
 };
 
-enum resolve_mode {
-	RESOLVE_TBD,	/* To Be Determined */
-	RESOLVE_PTR,	/* Resolving for Pointer */
-	RESOLVE_STRUCT_OR_ARRAY,	/* Resolving for struct/union
-					 * or array
-					 */
-};
-
 #define MAX_RESOLVE_DEPTH 32
 
 struct btf_sec_info {
@@ -254,7 +246,6 @@ struct btf_verifier_env {
 	u32 log_type_id;
 	u32 top_stack;
 	enum verifier_phase phase;
-	enum resolve_mode resolve_mode;
 };
 
 static const char * const btf_kind_str[NR_BTF_KINDS] = {
@@ -964,26 +955,7 @@ static void btf_verifier_env_free(struct btf_verifier_env *env)
 static bool env_type_is_resolve_sink(const struct btf_verifier_env *env,
 				     const struct btf_type *next_type)
 {
-	switch (env->resolve_mode) {
-	case RESOLVE_TBD:
-		/* int, enum or void is a sink */
-		return !btf_type_needs_resolve(next_type);
-	case RESOLVE_PTR:
-		/* int, enum, void, struct, array, func or func_proto is a sink
-		 * for ptr
-		 */
-		return !btf_type_is_modifier(next_type) &&
-			!btf_type_is_ptr(next_type);
-	case RESOLVE_STRUCT_OR_ARRAY:
-		/* int, enum, void, ptr, func or func_proto is a sink
-		 * for struct and array
-		 */
-		return !btf_type_is_modifier(next_type) &&
-			!btf_type_is_array(next_type) &&
-			!btf_type_is_struct(next_type);
-	default:
-		BUG();
-	}
+	return !btf_type_needs_resolve(next_type);
 }
 
 static bool env_type_is_resolved(const struct btf_verifier_env *env,
@@ -1010,13 +982,6 @@ static int env_stack_push(struct btf_verifier_env *env,
 	v->type_id = type_id;
 	v->next_member = 0;
 
-	if (env->resolve_mode == RESOLVE_TBD) {
-		if (btf_type_is_ptr(t))
-			env->resolve_mode = RESOLVE_PTR;
-		else if (btf_type_is_struct(t) || btf_type_is_array(t))
-			env->resolve_mode = RESOLVE_STRUCT_OR_ARRAY;
-	}
-
 	return 0;
 }
 
@@ -1038,7 +1003,7 @@ static void env_stack_pop_resolved(struct btf_verifier_env *env,
 	env->visit_states[type_id] = RESOLVED;
 }
 
-static const struct resolve_vertex *env_stack_peak(struct btf_verifier_env *env)
+static const struct resolve_vertex *env_stack_peek(struct btf_verifier_env *env)
 {
 	return env->top_stack ? &env->stack[env->top_stack - 1] : NULL;
 }
@@ -3030,9 +2995,8 @@ static int btf_resolve(struct btf_verifier_env *env,
 	const struct resolve_vertex *v;
 	int err = 0;
 
-	env->resolve_mode = RESOLVE_TBD;
 	env_stack_push(env, t, type_id);
-	while (!err && (v = env_stack_peak(env))) {
+	while (!err && (v = env_stack_peek(env))) {
 		env->log_type_id = v->type_id;
 		err = btf_type_ops(v->t)->resolve(env, v);
 	}
-- 
2.17.1