Commit d1a6edec authored by Stanislav Fomichev's avatar Stanislav Fomichev Committed by Daniel Borkmann
Browse files

bpf: Check attach_func_proto more carefully in check_return_code 



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.

Fixes: 69fd337a ("bpf: per-cgroup lsm flavor")
Reported-by: default avatar <syzbot+5cc0730bd4b4d2c5f152@syzkaller.appspotmail.com>
Signed-off-by: default avatarStanislav Fomichev <sdf@google.com>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Acked-by: default avatarMartin KaFai Lau <kafai@fb.com>
Link: https://lore.kernel.org/bpf/20220708175000.2603078-1-sdf@google.com
parent 32e0d9b3

kernel/bpf/verifier.c

+16 −5
Original line number Diff line number Diff line
@@ -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)

	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;

tools/testing/selftests/bpf/prog_tests/lsm_cgroup.c

+12 −0
Original line number Diff line number Diff line
@@ -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);

}

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

+6 −6
Original line number Diff line number Diff line
@@ -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;

}

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

0 → 100644
+14 −0
Original line number Diff line number Diff line 
// 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;

}