Re: [PATCH V3 1/2] selinux-testsuite: Add BPF tests

[Stephen Smalley <sds@tycho.nsa.gov>]

On 8/14/19 5:21 AM, Richard Haines wrote:
> This adds basic BPF tests for map and prog functions.
> 
> Signed-off-by: Richard Haines <richard_c_haines@btinternet.com>
> ---
> V2 Change - Split BPF code into bpf_common.c for others to use.
> V3 Changes - Correct style, Fix typos
> 
>   README.md              |  4 +-
>   defconfig              |  5 +++
>   policy/Makefile        |  4 ++
>   policy/test_bpf.te     | 77 +++++++++++++++++++++++++++++++++
>   tests/Makefile         |  4 ++
>   tests/bpf/.gitignore   |  2 +
>   tests/bpf/Makefile     | 12 ++++++
>   tests/bpf/bpf_common.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>   tests/bpf/bpf_test.c   | 82 +++++++++++++++++++++++++++++++++++
>   tests/bpf/test         | 58 +++++++++++++++++++++++++
>   10 files changed, 344 insertions(+), 1 deletion(-)
>   create mode 100644 policy/test_bpf.te
>   create mode 100644 tests/bpf/.gitignore
>   create mode 100644 tests/bpf/Makefile
>   create mode 100644 tests/bpf/bpf_common.c
>   create mode 100644 tests/bpf/bpf_test.c
>   create mode 100755 tests/bpf/test
> 
> diff --git a/README.md b/README.md
> index 26784f8..1396c8e 100644
> --- a/README.md
> +++ b/README.md
> @@ -51,6 +51,7 @@ similar dependencies):
>   * iptables _(to load the `iptables SECMARK` rules during `inet_socket` tests)_
>   * lksctp-tools-devel _(to build the SCTP test programs)_
>   * attr _(tools used by the overlayfs tests)_
> +* libbpf-devel _(tools used by the bpf tests)_
>   
>   On a modern Fedora system you can install these dependencies with the
>   following command:
> @@ -65,7 +66,8 @@ following command:
>   		netlabel_tools \
>   		iptables \
>   		lksctp-tools-devel \
> -		attr
> +		attr \
> +		libbpf-devel

Since we stipulate it as a dependency here, we shouldn't make the test 
conditional on the presence of the header.  Otherwise, the test may 
silently be skipped due to an oversight even on kernels/policies that 
support it.

>   
>   The testsuite requires a pre-existing base policy configuration of SELinux,
>   using either the old example policy or the reference policy as the baseline.
> diff --git a/defconfig b/defconfig
> index d7f0ea5..96f6443 100644
> --- a/defconfig
> +++ b/defconfig
> @@ -62,3 +62,8 @@ CONFIG_ANDROID_BINDER_IPC=y
>   # This will configure the Dynamically Allocated Binder Devices added
>   # to 5.0+ kernels:
>   CONFIG_ANDROID_BINDERFS=y
> +
> +# Test BPF + check in selinux_file_receive and selinux_binder_transfer_files.
> +# They are not required for SELinux operation itself.
> +CONFIG_BP=y

Typo in the config option name

> +CONFIG_BPF_SYSCALL=y
> diff --git a/policy/Makefile b/policy/Makefile
> index 305b572..16a4469 100644
> --- a/policy/Makefile
> +++ b/policy/Makefile
> @@ -71,6 +71,10 @@ ifeq ($(shell grep -q corenet_sctp_bind_all_nodes $(POLDEV)/include/kernel/coren
>   TARGETS += test_sctp.te
>   endif
>   
> +ifeq ($(shell grep -q bpf $(POLDEV)/include/support/all_perms.spt && echo true),true)
> +TARGETS += test_bpf.te
> +endif
> +
>   ifeq (x$(DISTRO),$(filter x$(DISTRO),xRHEL4 xRHEL5 xRHEL6))
>   TARGETS:=$(filter-out test_overlayfs.te test_mqueue.te, $(TARGETS))
>   endif
> diff --git a/policy/test_bpf.te b/policy/test_bpf.te
> new file mode 100644
> index 0000000..38b7729
> --- /dev/null
> +++ b/policy/test_bpf.te
> @@ -0,0 +1,77 @@
> +#
> +################# BPF selinux-testsuite policy module ######################
> +#
> +
> +attribute bpfdomain;
> +
> +################################### Main ###################################
> +type test_bpf_t;
> +domain_type(test_bpf_t)
> +unconfined_runs_test(test_bpf_t)
> +typeattribute test_bpf_t testdomain;
> +typeattribute test_bpf_t bpfdomain;
> +
> +allow test_bpf_t self:process { setrlimit };
> +allow test_bpf_t self:capability { sys_resource sys_admin };

The point (or at least part of it) of introducing the bpf access 
controls was to provide a middle ground between requiring CAP_SYS_ADMIN 
and allowing all processes to create/use these objects.  To truly test 
that, you may need to set the sysctl to allow unprivileged use and 
further there are still cases that require CAP_SYS_ADMIN even then, so 
the exact program/map matters.

> +allow test_bpf_t self:bpf { map_create map_read map_write prog_load prog_run };
> +
> +############################## Deny map_create #############################
> +type test_bpf_deny_map_create_t;
> +domain_type(test_bpf_deny_map_create_t)
> +unconfined_runs_test(test_bpf_deny_map_create_t)
> +typeattribute test_bpf_deny_map_create_t testdomain;
> +typeattribute test_bpf_deny_map_create_t bpfdomain;
> +
> +allow test_bpf_deny_map_create_t self:process { setrlimit };
> +allow test_bpf_deny_map_create_t self:capability { sys_resource sys_admin };
> +allow test_bpf_deny_map_create_t self:bpf { map_read map_write prog_load prog_run };
> +
> +############################## Deny map_read ##############################
> +type test_bpf_deny_map_read_t;
> +domain_type(test_bpf_deny_map_read_t)
> +unconfined_runs_test(test_bpf_deny_map_read_t)
> +typeattribute test_bpf_deny_map_read_t testdomain;
> +typeattribute test_bpf_deny_map_read_t bpfdomain;
> +
> +allow test_bpf_deny_map_read_t self:process { setrlimit };
> +allow test_bpf_deny_map_read_t self:capability { sys_resource sys_admin };
> +allow test_bpf_deny_map_read_t self:bpf { map_create map_write prog_load prog_run };
> +
> +############################## Deny map_write ##############################
> +type test_bpf_deny_map_write_t;
> +domain_type(test_bpf_deny_map_write_t)
> +unconfined_runs_test(test_bpf_deny_map_write_t)
> +typeattribute test_bpf_deny_map_write_t testdomain;
> +typeattribute test_bpf_deny_map_write_t bpfdomain;
> +
> +allow test_bpf_deny_map_write_t self:process { setrlimit };
> +allow test_bpf_deny_map_write_t self:capability { sys_resource sys_admin };
> +allow test_bpf_deny_map_write_t self:bpf { map_create map_read prog_load prog_run };
> +
> +############################## Deny prog_load ##############################
> +type test_bpf_deny_prog_load_t;
> +domain_type(test_bpf_deny_prog_load_t)
> +unconfined_runs_test(test_bpf_deny_prog_load_t)
> +typeattribute test_bpf_deny_prog_load_t testdomain;
> +typeattribute test_bpf_deny_prog_load_t bpfdomain;
> +
> +allow test_bpf_deny_prog_load_t self:process { setrlimit };
> +allow test_bpf_deny_prog_load_t self:capability { sys_resource sys_admin };
> +allow test_bpf_deny_prog_load_t self:bpf { map_create map_read map_write prog_run };
> +
> +############################## Deny prog_run ###############################
> +type test_bpf_deny_prog_run_t;
> +domain_type(test_bpf_deny_prog_run_t)
> +unconfined_runs_test(test_bpf_deny_prog_run_t)
> +typeattribute test_bpf_deny_prog_run_t testdomain;
> +typeattribute test_bpf_deny_prog_run_t bpfdomain;
> +
> +allow test_bpf_deny_prog_run_t self:process { setrlimit };
> +allow test_bpf_deny_prog_run_t self:capability { sys_resource sys_admin };
> +allow test_bpf_deny_prog_run_t self:bpf { map_create map_read map_write prog_load };
> +
> +#
> +############ Allow these domains to be entered from sysadm domain ############
> +#
> +miscfiles_domain_entry_test_files(bpfdomain)
> +userdom_sysadm_entry_spec_domtrans_to(bpfdomain)
> diff --git a/tests/Makefile b/tests/Makefile
> index 63aa325..2717452 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -42,6 +42,10 @@ ifeq ($(shell grep -q binder $(POLDEV)/include/support/all_perms.spt && test -e
>   SUBDIRS += binder
>   endif
>   
> +ifeq ($(shell grep -q bpf $(POLDEV)/include/support/all_perms.spt && test -e $(INCLUDEDIR)/bpf/bpf.h && echo true),true)
> +SUBDIRS += bpf
> +endif

Don't make it conditional on the header since that is stated build 
dependency.  Do make it or the test itself conditional on kernel version 
so that it doesn't break on kernels that predate the bpf access controls.

> +
>   ifeq ($(shell grep "^SELINUX_INFINIBAND_ENDPORT_TEST=" infiniband_endport/ibendport_test.conf | cut -d'=' -f 2),1)
>   SUBDIRS += infiniband_endport
>   endif
> diff --git a/tests/bpf/.gitignore b/tests/bpf/.gitignore
> new file mode 100644
> index 0000000..1919ff8
> --- /dev/null
> +++ b/tests/bpf/.gitignore
> @@ -0,0 +1,2 @@
> +bpf_test
> +bpf_common
> diff --git a/tests/bpf/Makefile b/tests/bpf/Makefile
> new file mode 100644
> index 0000000..78ae9db
> --- /dev/null
> +++ b/tests/bpf/Makefile
> @@ -0,0 +1,12 @@
> +TARGETS = bpf_test
> +DEPS = bpf_common.c
> +
> +LDLIBS += -lselinux -lbpf
> +CFLAGS += -DHAVE_BPF
> +
> +all: $(TARGETS)
> +
> +clean:
> +	rm -f $(TARGETS)
> +
> +$(TARGETS): $(DEPS)
> diff --git a/tests/bpf/bpf_common.c b/tests/bpf/bpf_common.c
> new file mode 100644
> index 0000000..3ac47be
> --- /dev/null
> +++ b/tests/bpf/bpf_common.c
> @@ -0,0 +1,97 @@
> +#include <stdio.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +
> +#if HAVE_BPF /* Start HAVE_BPF */

Not sure what the point of this #if is given that you define it 
unconditionally in the Makefile and you made the package that provides 
the header a build dependency.

> +#include <bpf/bpf.h>
> +#include <linux/bpf.h>
> +#include <sys/resource.h>
> +
> +/* edited eBPF instruction library */
> +/* Short form of mov, dst_reg = imm32 */
> +#define BPF_MOV64_IMM(DST, IMM)				\
> +	((struct bpf_insn) {				\
> +		.code  = BPF_ALU64 | BPF_MOV | BPF_K,	\
> +			 .dst_reg = DST,				\
> +				    .src_reg = 0,				\
> +					       .off   = 0,				\
> +							.imm   = IMM })
> +
> +/* Program exit */
> +#define BPF_EXIT_INSN()				\
> +	((struct bpf_insn) {			\
> +		.code  = BPF_JMP | BPF_EXIT,	\
> +			 .dst_reg = 0,			\
> +				    .src_reg = 0,			\
> +					       .off   = 0,			\
> +							.imm   = 0 })

That's ugly but I guess Paul wants it that way.

> +
> +int create_bpf_map(void)
> +{
> +	int map_fd, key;
> +	long long value = 0;
> +
> +	map_fd = bpf_create_map(BPF_MAP_TYPE_ARRAY, sizeof(key),
> +				sizeof(value), 256, 0);
> +	if (map_fd < 0) {
> +		fprintf(stderr, "Failed to create BPF map: %s\n",
> +			strerror(errno));
> +		return -1;
> +	}
> +
> +	return map_fd;
> +}
> +
> +int create_bpf_prog(void)
> +{
> +	int prog_fd;
> +	size_t insns_cnt;
> +
> +	struct bpf_insn prog[] = {
> +		BPF_MOV64_IMM(BPF_REG_0, 1),
> +		BPF_EXIT_INSN(),
> +	};
> +	insns_cnt = sizeof(prog) / sizeof(struct bpf_insn);
> +
> +	prog_fd = bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER, prog,
> +				   insns_cnt, "GPL", 0, NULL, 0);
> +	if (prog_fd < 0) {
> +		fprintf(stderr, "Failed to load BPF prog: %s\n",
> +			strerror(errno));
> +		return -1;
> +	}
> +
> +	return prog_fd;
> +}
> +
> +void bpf_setrlimit(void)
> +{
> +	int result;
> +	struct rlimit r = { RLIM_INFINITY, RLIM_INFINITY };
> +
> +	result = setrlimit(RLIMIT_MEMLOCK, &r);
> +	if (result < 0) {
> +		fprintf(stderr, "Failed to set resource limit: %s\n",
> +			strerror(errno));
> +		exit(-1);
> +	}
> +}
> +
> +#else
> +int create_bpf_map(void)
> +{
> +	fprintf(stderr, "BPF map not supported\n");
> +	return -1;
> +}
> +
> +int create_bpf_prog(void)
> +{
> +	fprintf(stderr, "BPF prog not supported\n");
> +	return -1;
> +}
> +
> +void bpf_setrlimit(void)
> +{
> +}
> +#endif /* End HAVE_BPF */
> diff --git a/tests/bpf/bpf_test.c b/tests/bpf/bpf_test.c
> new file mode 100644
> index 0000000..b866651
> --- /dev/null
> +++ b/tests/bpf/bpf_test.c
> @@ -0,0 +1,82 @@
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <selinux/selinux.h>
> +
> +int create_bpf_map(void);
> +int create_bpf_prog(void);
> +void bpf_setrlimit(void);
> +
> +static void usage(char *progname)
> +{
> +	fprintf(stderr,
> +		"usage:  %s [-m|-p] [-v]\n"
> +		"Where:\n\t"
> +		"-m    Create BPF map fd\n\t"
> +		"-p    Create BPF prog fd\n\t"
> +		"-v Print information.\n", progname);
> +	exit(-1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	int opt, result, fd, bpf_fd_type = 0;
> +	bool verbose = false;
> +	char *context;
> +
> +	while ((opt = getopt(argc, argv, "mpv")) != -1) {
> +		switch (opt) {
> +		case 'm':
> +			bpf_fd_type = 1;
> +			break;
> +		case 'p':
> +			bpf_fd_type = 2;

No magic constants please, use a #define or enum with symbolic names.

> +			break;
> +		case 'v':
> +			verbose = true;
> +			break;
> +		default:
> +			usage(argv[0]);
> +		}
> +	}
> +
> +	result = getcon(&context);
> +	if (result < 0) {
> +		fprintf(stderr, "Failed to obtain SELinux context\n");
> +		exit(-1);
> +	}
> +
> +	if (verbose)
> +		printf("Process context:\n\t%s\n", context);
> +
> +	free(context);
> +
> +	/* If BPF enabled, then need to set limits */
> +	bpf_setrlimit();
> +
> +	switch (bpf_fd_type) {
> +	case 1:
> +		if (verbose)
> +			printf("Creating BPF map\n");
> +
> +		fd = create_bpf_map();
> +		break;
> +	case 2:
> +		if (verbose)
> +			printf("Creating BPF prog\n");
> +
> +		fd = create_bpf_prog();
> +		break;
> +	default:
> +		usage(argv[0]);
> +	}
> +
> +	if (fd < 0)
> +		return bpf_fd_type;
> +
> +	close(fd);
> +	return 0;
> +}
> diff --git a/tests/bpf/test b/tests/bpf/test
> new file mode 100755
> index 0000000..1d41d72
> --- /dev/null
> +++ b/tests/bpf/test
> @@ -0,0 +1,58 @@
> +#!/usr/bin/perl
> +use Test::More;
> +
> +BEGIN {
> +    $basedir = $0;
> +    $basedir =~ s|(.*)/[^/]*|$1|;
> +
> +    # allow info to be shown during tests
> +    $v = $ARGV[0];
> +    if ($v) {
> +        if ( $v ne "-v" ) {
> +            plan skip_all => "Invalid option (use -v)";
> +        }
> +    }
> +    else {
> +        $v = " ";
> +    }
> +
> +    plan tests => 7;
> +}
> +
> +#
> +################ Core BPF Tests #######################
> +#
> +# BPF map
> +$result = system "runcon -t test_bpf_t $basedir/bpf_test -m $v";
> +ok( $result eq 0 );
> +
> +# BPF prog
> +$result = system "runcon -t test_bpf_t $basedir/bpf_test -p $v";
> +ok( $result eq 0 );
> +
> +# Deny map_create permission
> +$result =
> +  system "runcon -t test_bpf_deny_map_create_t $basedir/bpf_test -m $v 2>&1";
> +ok( $result >> 8 eq 1 );
> +
> +# Deny map_read permission
> +$result =
> +  system "runcon -t test_bpf_deny_map_read_t $basedir/bpf_test -m $v 2>&1";
> +ok( $result >> 8 eq 1 );
> +
> +# Deny map_write permission
> +$result =
> +  system "runcon -t test_bpf_deny_map_write_t $basedir/bpf_test -m $v 2>&1";
> +ok( $result >> 8 eq 1 );
> +
> +# Deny prog_load permission
> +$result =
> +  system "runcon -t test_bpf_deny_prog_load_t $basedir/bpf_test -p $v 2>&1";
> +ok( $result >> 8 eq 2 );
> +
> +# Deny prog_run permission
> +$result =
> +  system "runcon -t test_bpf_deny_prog_run_t $basedir/bpf_test -p $v 2>&1";
> +ok( $result >> 8 eq 2 );
> +
> +exit;

Tests 8 and 9 failed for me,
bpf/test .................... 1/11 BPF map not supported
sendmsg: Bad file descriptor

#   Failed test at bpf/test line 78.
BPF prog not supported
connect: Connection refused

#   Failed test at bpf/test line 82.
BPF prog not supported
connect: Connection refused
BPF map not supported
connect: Connection refused
# Looks like you failed 2 tests of 11.
bpf/test .................... Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/11 subtests