[bpf,v2] bpf: relax return code check for subprograms

Commit Message

Dmitrii Banshchikov Nov. 13, 2020, 5:17 p.m. UTC

Currently verifier enforces return code checks for subprograms in the
same manner as it does for program entry points. This prevents returning
arbitrary scalar values from subprograms. Scalar type of returned values
is checked by btf_prepare_func_args() and hence it should be safe to
allow only scalars for now. Relax return code checks for subprograms and
allow any correct scalar values.

Signed-off-by: Dmitrii Banshchikov <[e-mail]>
Fixes: 51c39bb1d5d10 (bpf: Introduce function-by-function verification)
---
v1 -> v2:
 - Move is_subprog flag determination to check_return_code()
 - Remove unneeded intermediate function from tests
 - Use __noinline instead of __attribute__((noinline)) in tests

 kernel/bpf/verifier.c                         | 15 +++++++++++++--
 .../bpf/prog_tests/test_global_funcs.c        |  1 +
 .../selftests/bpf/progs/test_global_func8.c   | 19 +++++++++++++++++++
 3 files changed, 33 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/test_global_func8.c

Comments

Andrii Nakryiko Nov. 13, 2020, 8:06 p.m. UTC | #1

On Fri, Nov 13, 2020 at 9:18 AM Dmitrii Banshchikov <[e-mail]> wrote:
>
> Currently verifier enforces return code checks for subprograms in the
> same manner as it does for program entry points. This prevents returning
> arbitrary scalar values from subprograms. Scalar type of returned values
> is checked by btf_prepare_func_args() and hence it should be safe to
> allow only scalars for now. Relax return code checks for subprograms and
> allow any correct scalar values.
>
> Signed-off-by: Dmitrii Banshchikov <[e-mail]>
> Fixes: 51c39bb1d5d10 (bpf: Introduce function-by-function verification)
> ---

LGTM!

Acked-by: Andrii Nakryiko <[e-mail]>

> v1 -> v2:
>  - Move is_subprog flag determination to check_return_code()
>  - Remove unneeded intermediate function from tests
>  - Use __noinline instead of __attribute__((noinline)) in tests
>
>  kernel/bpf/verifier.c                         | 15 +++++++++++++--
>  .../bpf/prog_tests/test_global_funcs.c        |  1 +
>  .../selftests/bpf/progs/test_global_func8.c   | 19 +++++++++++++++++++
>  3 files changed, 33 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/progs/test_global_func8.c
>

[...]

[e-mail] Nov. 14, 2020, 4:40 p.m. UTC | #2

Hello:

This patch was applied to bpf/bpf.git (refs/heads/master):

On Fri, 13 Nov 2020 17:17:56 +0000 you wrote:
> Currently verifier enforces return code checks for subprograms in the
> same manner as it does for program entry points. This prevents returning
> arbitrary scalar values from subprograms. Scalar type of returned values
> is checked by btf_prepare_func_args() and hence it should be safe to
> allow only scalars for now. Relax return code checks for subprograms and
> allow any correct scalar values.
> 
> [...]

Here is the summary with links:
  - [bpf,v2] bpf: relax return code check for subprograms
    https://git.kernel.org/bpf/bpf/c/f782e2c300a7

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6204ec705d80..1388bf733071 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7786,9 +7786,11 @@  static int check_return_code(struct bpf_verifier_env *env)
 	struct tnum range = tnum_range(0, 1);
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
 	int err;
+	const bool is_subprog = env->cur_state->frame[0]->subprogno;
 
 	/* LSM and struct_ops func-ptr's return type could be "void" */
-	if ((prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
+	if (!is_subprog &&
+	    (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
 	     prog_type == BPF_PROG_TYPE_LSM) &&
 	    !prog->aux->attach_func_proto->type)
 		return 0;
@@ -7808,6 +7810,16 @@  static int check_return_code(struct bpf_verifier_env *env)
 		return -EACCES;
 	}
 
+	reg = cur_regs(env) + BPF_REG_0;
+	if (is_subprog) {
+		if (reg->type != SCALAR_VALUE) {
+			verbose(env, "At subprogram exit the register R0 is not a scalar value (%s)\n",
+				reg_type_str[reg->type]);
+			return -EINVAL;
+		}
+		return 0;
+	}
+
 	switch (prog_type) {
 	case BPF_PROG_TYPE_CGROUP_SOCK_ADDR:
 		if (env->prog->expected_attach_type == BPF_CGROUP_UDP4_RECVMSG ||
@@ -7861,7 +7873,6 @@  static int check_return_code(struct bpf_verifier_env *env)
 		return 0;
 	}
 
-	reg = cur_regs(env) + BPF_REG_0;
 	if (reg->type != SCALAR_VALUE) {
 		verbose(env, "At program exit the register R0 is not a known value (%s)\n",
 			reg_type_str[reg->type]);
diff --git a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
index 193002b14d7f..32e4348b714b 100644
--- a/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
+++ b/tools/testing/selftests/bpf/prog_tests/test_global_funcs.c
@@ -60,6 +60,7 @@  void test_test_global_funcs(void)
 		{ "test_global_func5.o" , "expected pointer to ctx, but got PTR" },
 		{ "test_global_func6.o" , "modified ctx ptr R2" },
 		{ "test_global_func7.o" , "foo() doesn't return scalar" },
+		{ "test_global_func8.o" },
 	};
 	libbpf_print_fn_t old_print_fn = NULL;
 	int err, i, duration = 0;
diff --git a/tools/testing/selftests/bpf/progs/test_global_func8.c b/tools/testing/selftests/bpf/progs/test_global_func8.c
new file mode 100644
index 000000000000..d55a6544b1ab
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_global_func8.c
@@ -0,0 +1,19 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2020 Facebook */
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+__noinline int foo(struct __sk_buff *skb)
+{
+	return bpf_get_prandom_u32();
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	if (!foo(skb))
+		return 0;
+
+	return 1;
+}