[PATCH bpf-next v3] bpf: check attach_func_proto more carefully in check_return_code

[PATCH bpf-next v3] bpf: check attach_func_proto more carefully in check_return_code

[Stanislav Fomichev]

Syzkaller reports the following crash:
RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610

With the following reproducer:
bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)

Because we don't enforce expected_attach_type for XDP programs,
we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP'
part in check_return_code and follow up with testing
`prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto`
is NULL.

Add explicit prog_type check for the "Note, BPF_LSM_CGROUP that
attach ..." condition. Also, don't skip return code check for
LSM/STRUCT_OPS.

The above actually brings an issue with existing selftest which
tries to return EPERM from void inet_csk_clone. Fix the
test (and move called_socket_clone to make sure it's not
incremented in case of an error) and add a new one to explicitly
verify this condition.

v3:
- Martin: handle expected_attach_type for BPF_PROG_TYPE_STRUCT_OPS as well

v2:
- Martin: don't add new helper, check prog_type instead
- Martin: check expected_attach_type as well at the function entry
- Update selftest to verify this condition

Fixes: 69fd337a975c ("bpf: per-cgroup lsm flavor")
Reported-by: syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/verifier.c                         | 21 ++++++++++++++-----
 .../selftests/bpf/prog_tests/lsm_cgroup.c     | 12 +++++++++++
 .../testing/selftests/bpf/progs/lsm_cgroup.c  | 12 +++++------
 .../selftests/bpf/progs/lsm_cgroup_nonvoid.c  | 14 +++++++++++++
 4 files changed, 48 insertions(+), 11 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index df3ec6b05f05..e3cf6194c24f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -10444,11 +10444,21 @@ static int check_return_code(struct bpf_verifier_env *env)
 	const bool is_subprog = frame->subprogno;
 
 	/* LSM and struct_ops func-ptr's return type could be "void" */
-	if (!is_subprog &&
-	    (prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
-	     prog_type == BPF_PROG_TYPE_LSM) &&
-	    !prog->aux->attach_func_proto->type)
-		return 0;
+	if (!is_subprog) {
+		switch (prog_type) {
+		case BPF_PROG_TYPE_LSM:
+			if (prog->expected_attach_type == BPF_LSM_CGROUP)
+				/* See below, can be 0 or 0-1 depending on hook. */
+				break;
+			fallthrough;
+		case BPF_PROG_TYPE_STRUCT_OPS:
+			if (!prog->aux->attach_func_proto->type)
+				return 0;
+			break;
+		default:
+			break;
+		}
+	}
 
 	/* eBPF calling convention is such that R0 is used
 	 * to return the value from eBPF program.
@@ -10572,6 +10582,7 @@ static int check_return_code(struct bpf_verifier_env *env)
 	if (!tnum_in(range, reg->var_off)) {
 		verbose_invalid_scalar(env, reg, &range, "program exit", "R0");
 		if (prog->expected_attach_type == BPF_LSM_CGROUP &&
+		    prog_type == BPF_PROG_TYPE_LSM &&
 		    !prog->aux->attach_func_proto->type)
 			verbose(env, "Note, BPF_LSM_CGROUP that attach to void LSM hooks can't modify return value!\n");
 		return -EINVAL;
diff --git a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
index c542d7e80a5b..1102e4f42d2d 100644
--- a/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c
@@ -6,6 +6,7 @@
 #include <bpf/btf.h>
 
 #include "lsm_cgroup.skel.h"
+#include "lsm_cgroup_nonvoid.skel.h"
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
 
@@ -293,9 +294,20 @@ static void test_lsm_cgroup_functional(void)
 	lsm_cgroup__destroy(skel);
 }
 
+static void test_lsm_cgroup_nonvoid(void)
+{
+	struct lsm_cgroup_nonvoid *skel = NULL;
+
+	skel = lsm_cgroup_nonvoid__open_and_load();
+	ASSERT_NULL(skel, "open succeeds");
+	lsm_cgroup_nonvoid__destroy(skel);
+}
+
 void test_lsm_cgroup(void)
 {
 	if (test__start_subtest("functional"))
 		test_lsm_cgroup_functional();
+	if (test__start_subtest("nonvoid"))
+		test_lsm_cgroup_nonvoid();
 	btf__free(btf);
 }
diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup.c b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
index 89f3b1e961a8..4f2d60b87b75 100644
--- a/tools/testing/selftests/bpf/progs/lsm_cgroup.c
+++ b/tools/testing/selftests/bpf/progs/lsm_cgroup.c
@@ -156,25 +156,25 @@ int BPF_PROG(socket_clone, struct sock *newsk, const struct request_sock *req)
 {
 	int prio = 234;
 
-	called_socket_clone++;
-
 	if (!newsk)
 		return 1;
 
 	/* Accepted request sockets get a different priority. */
 	if (bpf_setsockopt(newsk, SOL_SOCKET, SO_PRIORITY, &prio, sizeof(prio)))
-		return 0; /* EPERM */
+		return 1;
 
 	/* Make sure bpf_getsockopt is allowed and works. */
 	prio = 0;
 	if (bpf_getsockopt(newsk, SOL_SOCKET, SO_PRIORITY, &prio, sizeof(prio)))
-		return 0; /* EPERM */
+		return 1;
 	if (prio != 234)
-		return 0; /* EPERM */
+		return 1;
 
 	/* Can access cgroup local storage. */
 	if (!test_local_storage())
-		return 0; /* EPERM */
+		return 1;
+
+	called_socket_clone++;
 
 	return 1;
 }
diff --git a/tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c b/tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
new file mode 100644
index 000000000000..6cb0f161f417
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/lsm_cgroup_nonvoid.c
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+SEC("lsm_cgroup/inet_csk_clone")
+int BPF_PROG(nonvoid_socket_clone, struct sock *newsk, const struct request_sock *req)
+{
+	/* Can not return any errors from void LSM hooks. */
+	return 0;
+}
-- 
2.37.0.rc0.161.g10f37bed90-goog

Re: [PATCH bpf-next v3] bpf: check attach_func_proto more carefully in check_return_code

[Martin KaFai Lau]

On Fri, Jul 08, 2022 at 10:50:00AM -0700, Stanislav Fomichev wrote:
> Syzkaller reports the following crash:
> RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
> RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
> RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610
> 
> With the following reproducer:
> bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)
> 
> Because we don't enforce expected_attach_type for XDP programs,
> we end up in hitting 'if (prog->expected_attach_type == BPF_LSM_CGROUP'
> part in check_return_code and follow up with testing
> `prog->aux->attach_func_proto->type`, but `prog->aux->attach_func_proto`
> is NULL.
> 
> Add explicit prog_type check for the "Note, BPF_LSM_CGROUP that
> attach ..." condition. Also, don't skip return code check for
> LSM/STRUCT_OPS.
> 
> The above actually brings an issue with existing selftest which
> tries to return EPERM from void inet_csk_clone. Fix the
> test (and move called_socket_clone to make sure it's not
> incremented in case of an error) and add a new one to explicitly
> verify this condition.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf-next v3] bpf: check attach_func_proto more carefully in check_return_code

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri,  8 Jul 2022 10:50:00 -0700 you wrote:
> Syzkaller reports the following crash:
> RIP: 0010:check_return_code kernel/bpf/verifier.c:10575 [inline]
> RIP: 0010:do_check kernel/bpf/verifier.c:12346 [inline]
> RIP: 0010:do_check_common+0xb3d2/0xd250 kernel/bpf/verifier.c:14610
> 
> With the following reproducer:
> bpf$PROG_LOAD_XDP(0x5, &(0x7f00000004c0)={0xd, 0x3, &(0x7f0000000000)=ANY=[@ANYBLOB="1800000000000019000000000000000095"], &(0x7f0000000300)='GPL\x00', 0x0, 0x0, 0x0, 0x0, 0x0, '\x00', 0x0, 0x2b, 0xffffffffffffffff, 0x8, 0x0, 0x0, 0x10, 0x0}, 0x80)
> 
> [...]

Here is the summary with links:
  - [bpf-next,v3] bpf: check attach_func_proto more carefully in check_return_code
    https://git.kernel.org/bpf/bpf-next/c/d1a6edecc1fd

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