bpf: relax return code check for subprograms

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 <[email protected]>
Fixes: 51c39bb (bpf: Introduce function-by-function verification)

Author: ubique

kernel/bpf/verifier.c

@@ -7791,7 +7791,7 @@ static int check_ld_abs(struct bpf_verifier_env *env, struct bpf_insn *insn)
 	return 0;
 }
 
-static int check_return_code(struct bpf_verifier_env *env)
+static int check_return_code(struct bpf_verifier_env *env, bool is_subprog)
 {
 	struct tnum enforce_attach_type_range = tnum_unknown;
 	const struct bpf_prog *prog = env->prog;
@@ -7801,10 +7801,12 @@ static int check_return_code(struct bpf_verifier_env *env)
 	int err;
 
 	/* LSM and struct_ops func-ptr's return type could be "void" */
-	if ((prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
-	     prog_type == BPF_PROG_TYPE_LSM) &&
-	    !prog->aux->attach_func_proto->type)
-		return 0;
+	if (!is_subprog) {
+		if ((prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
+		     prog_type == BPF_PROG_TYPE_LSM) &&
+		    !prog->aux->attach_func_proto->type)
+			return 0;
+	}
 
 	/* eBPF calling convetion is such that R0 is used
 	 * to return the value from eBPF program.
@@ -7821,6 +7823,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 ||
@@ -7874,7 +7886,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]);
@@ -9266,6 +9277,7 @@ static int do_check(struct bpf_verifier_env *env)
 	int insn_cnt = env->prog->len;
 	bool do_print_state = false;
 	int prev_insn_idx = -1;
+	const bool is_subprog = env->cur_state->frame[0]->subprogno;
 
 	for (;;) {
 		struct bpf_insn *insn;
@@ -9530,7 +9542,7 @@ static int do_check(struct bpf_verifier_env *env)
 				if (err)
 					return err;
 
-				err = check_return_code(env);
+				err = check_return_code(env, is_subprog);
 				if (err)
 					return err;
 process_bpf_exit:

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;

tools/testing/selftests/bpf/progs/test_global_func8.c

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2020 Facebook */
+#include <stddef.h>
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+__attribute__ ((noinline))
+int bar(struct __sk_buff *skb)
+{
+	return bpf_get_prandom_u32();
+}
+
+static __always_inline int foo(struct __sk_buff *skb)
+{
+	if (!bar(skb))
+		return 0;
+
+	return 1;
+}
+
+SEC("cgroup_skb/ingress")
+int test_cls(struct __sk_buff *skb)
+{
+	return foo(skb);
+}