netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrii Nakryiko <andriin@fb.com>
To: <andrii.nakryiko@gmail.com>, <ast@fb.com>, <daniel@iogearbox.net>,
	<bpf@vger.kernel.org>, <netdev@vger.kernel.org>,
	<kernel-team@fb.com>
Cc: Andrii Nakryiko <andriin@fb.com>, Martin KaFai Lau <kafai@fb.com>
Subject: [PATCH bpf] bpf: fix BTF verifier size resolution logic
Date: Wed, 10 Jul 2019 01:08:40 -0700	[thread overview]
Message-ID: <20190710080840.2613160-1-andriin@fb.com> (raw)

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@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.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


             reply	other threads:[~2019-07-10  8:08 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-10  8:08 Andrii Nakryiko [this message]
2019-07-11  0:16 ` [PATCH bpf] bpf: fix BTF verifier size resolution logic Yonghong Song
2019-07-11  0:29   ` Andrii Nakryiko
2019-07-11  0:36     ` Yonghong Song
2019-07-11  1:45       ` Andrii Nakryiko
2019-07-11  1:53         ` Yonghong Song
2019-07-11  4:54           ` Andrii Nakryiko
2019-07-11  4:14         ` Yonghong Song
2019-07-11  4:56           ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190710080840.2613160-1-andriin@fb.com \
    --to=andriin@fb.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=ast@fb.com \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kafai@fb.com \
    --cc=kernel-team@fb.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).