On Wed, Apr 12, 2023 at 11:23 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Tue, Apr 11, 2023 at 09:32:57PM -0700, Andrii Nakryiko wrote:
> > Add selftests that goes over every known map type and validates that
> > a combination of privileged/unprivileged modes and allow/reject/pass-through
> > LSM policy decisions behave as expected.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
> > .../selftests/bpf/prog_tests/lsm_map_create.c | 143 ++++++++++++++++++
> > .../selftests/bpf/progs/lsm_map_create.c | 32 ++++
> > tools/testing/selftests/bpf/test_progs.h | 6 +
> > 3 files changed, 181 insertions(+)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/lsm_map_create.c
> > create mode 100644 tools/testing/selftests/bpf/progs/lsm_map_create.c
> >
[...]
> > + ret = libbpf_probe_bpf_map_type(map_type, NULL);
> > + ASSERT_EQ(ret, is_map_priv ? 0 : 1, "default_unpriv_mode");
> > +
> > + /* allow any map creation for our thread */
> > + skel->bss->decision = 1;
> > + ret = libbpf_probe_bpf_map_type(map_type, NULL);
> > + ASSERT_EQ(ret, 1, "lsm_allow_unpriv_mode");
> > +
> > + /* reject any map creation for our thread */
> > + skel->bss->decision = -1;
> > + ret = libbpf_probe_bpf_map_type(map_type, NULL);
> > + ASSERT_EQ(ret, 0, "lsm_reject_unpriv_mode");
> > +
> > + /* restore privileges, but keep reject LSM policy */
> > + if (!ASSERT_OK(restore_priv_caps(orig_caps), "restore_caps"))
> > + goto cleanup;
> > +
> > +skip_if_needs_btf:
> > + /* even with all caps map create will fail */
> > + skel->bss->decision = -1;
> > + ret = libbpf_probe_bpf_map_type(map_type, NULL);
> > + ASSERT_EQ(ret, 0, "lsm_reject_priv_mode");
> > + }
> > +
> > +cleanup:
> > + btf__free(btf);
> > + lsm_map_create__destroy(skel);
> > +}
>
> This test looks good! One meta-comment about testing would be: are you
> sure each needs to be ASSERT instead of EXPECT? (i.e. should forward
> progress through this test always be aborted when a check fails?)
>
it's our custom BPF selftests ASSERTs, they don't really do assert()
and panic, they really are just a check (so I'm guessing they have
EXPECT semantics you are referring to). And if check doesn't pass, we
just set a flag notifying our own custom test runner that test failed
and proceed.
So in short, it already behaves like you would want with EXPECT. We
just don't use kselftests's ASSERTs and EXPECTs.
> > diff --git a/tools/testing/selftests/bpf/progs/lsm_map_create.c b/tools/testing/selftests/bpf/progs/lsm_map_create.c
> > new file mode 100644
> > index 000000000000..093825c68459
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/lsm_map_create.c
> > @@ -0,0 +1,32 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
> > +
> > +#include "vmlinux.h"
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include <errno.h>
> > +
> > +char _license[] SEC("license") = "GPL";
> > +
> > +int my_tid;
> > +/* LSM enforcement:
> > + * - 0, delegate to kernel;
> > + * - 1, allow;
> > + * - -1, reject.
> > + */
> > +int decision;
> > +
> > +SEC("lsm/bpf_map_create_security")
> > +int BPF_PROG(allow_unpriv_maps, union bpf_attr *attr)
> > +{
> > + if (!my_tid || (u32)bpf_get_current_pid_tgid() != my_tid)
> > + return 0; /* keep processing LSM hooks */
> > +
> > + if (decision == 0)
> > + return 0;
> > +
> > + if (decision > 0)
> > + return 1; /* allow */
> > +
> > + return -EPERM;
> > +}
> > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > index 10ba43250668..12f9c6652d40 100644
> > --- a/tools/testing/selftests/bpf/test_progs.h
> > +++ b/tools/testing/selftests/bpf/test_progs.h
> > @@ -23,6 +23,7 @@ typedef __u16 __sum16;
> > #include <linux/perf_event.h>
> > #include <linux/socket.h>
> > #include <linux/unistd.h>
> > +#include <sys/syscall.h>
> >
> > #include <sys/ioctl.h>
> > #include <sys/wait.h>
> > @@ -176,6 +177,11 @@ void test__skip(void);
> > void test__fail(void);
> > int test__join_cgroup(const char *path);
> >
> > +static inline int gettid(void)
> > +{
> > + return syscall(SYS_gettid);
> > +}
> > +
> > #define PRINT_FAIL(format...) \
> > ({ \
> > test__fail(); \
> > --
> > 2.34.1
> >
>
> --
> Kees Cook
