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

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

On 9/16/19 1:54 PM, Richard Haines wrote:
> On Fri, 2019-09-13 at 13:39 -0400, Stephen Smalley wrote:
>> 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.
> 
> Fixed
> 
>>
>>>    
>>>    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
> 
> Fixed
>>
>>> +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.
> 
> I have removed the sys_admin perm as the Fedora default is:
> sysctl kernel.unprivileged_bpf_disabled
> kernel.unprivileged_bpf_disabled = 0
> 
> I've also found a map & prog type that requires sys_admin:
> BPF_MAP_TYPE_LPM_TRIE and BPF_PROG_TYPE_UNSPEC
> These tested ok.
> 
> Is it worth setting kernel.unprivileged_bpf_disabled = 1 and testing as
> it can only be reset by a reboot of the system, if so I'll add a
> message after tests.

I don't think so; we are primarily interested in testing the new SELinux 
BPF controls, not sys_admin, here.

> 
>>
>>> +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.
> 
> I've set this for kernel 4.4 as that supports kernel.unprivileged_bpf_disabled
> 
>>
>>> +
>>>    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.
> 
> 
> I've reworked the tests to remove this in bpf_common.c
> 
>>
>>> +#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.
> Fixed
>>
>>> +			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
> 
> Hopefully this is fixed.
> 
> Note when I send the updated patches I'll include the Binder BPF
> support as Patch 3.
>>
>>
> 
> 
>