[PATCH v2 bpf-next 0/2] bpf: refine kernel.unpriviliged_bpf_disabled behaviour

[PATCH v2 bpf-next 0/2] bpf: refine kernel.unpriviliged_bpf_disabled behaviour

[Alan Maguire]

Unprivileged BPF disabled (kernel.unprivileged_bpf_disabled >= 1)
is the default in most cases now; when set, the BPF system call is
blocked for users without CAP_BPF/CAP_SYS_ADMIN.  In some cases
however, it makes sense to split activities between capability-requiring
ones - such as program load/attach - and those that might not require
capabilities such as reading perf/ringbuf events, reading or
updating BPF map configuration etc.  One example of this sort of
approach is a service that loads a BPF program, and a user-space
program that interacts with it.  

Here - rather than blocking all BPF syscall commands - unprivileged
BPF disabled blocks the key object-creating commands (prog load,
map load).  Discussion has alluded to this idea in the past [1],
and Alexei mentioned it was also discussed at LSF/MM/BPF this year.

Changes since RFC [2]:

- widened scope of commands unprivileged BPF disabled allows
  (Alexei, patch 1)
- removed restrictions on map types for lookup, update, delete
  (Alexei, patch 1)
- removed kernel CONFIG parameter controlling unprivilged bpf disabled
  change (Alexei, patch 1)
- widened test scope to cover most BPF syscall commands, with positive
  and negative subtests

[1] https://lore.kernel.org/bpf/CAADnVQLTBhCTAx1a_nev7CgMZxv1Bb7ecz1AFRin8tHmjPREJA@mail.gmail.com/
[2] https://lore.kernel.org/bpf/20220511163604.5kuczj6jx3ec5qv6@MBP-98dd607d3435.dhcp.thefacebook.com/T/#mae65f35a193279e718f37686da636094d69b96ee

Alan Maguire (2):
  bpf: refine kernel.unpriviliged_bpf_disabled behaviour
  selftests/bpf: add tests verifying unprivileged bpf behaviour

 kernel/bpf/syscall.c                          |  14 +-
 .../bpf/prog_tests/unpriv_bpf_disabled.c      | 308 ++++++++++++++++++
 .../bpf/progs/test_unpriv_bpf_disabled.c      |  83 +++++
 3 files changed, 404 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c

-- 
2.27.0

[PATCH v2 bpf-next 1/2] bpf: refine kernel.unpriviliged_bpf_disabled behaviour

[Alan Maguire]

With unprivileged BPF disabled, all cmds associated with the BPF syscall
are blocked to users without CAP_BPF/CAP_SYS_ADMIN.  However there are
use cases where we may wish to allow interactions with BPF programs
without being able to load and attach them.  So for example, a process
with required capabilities loads/attaches a BPF program, and a process
with less capabilities interacts with it; retrieving perf/ring buffer
events, modifying map-specified config etc.  With all BPF syscall
commands blocked as a result of unprivileged BPF being disabled,
this mode of interaction becomes impossible for processes without
CAP_BPF.

As Alexei notes

"The bpf ACL model is the same as traditional file's ACL.
The creds and ACLs are checked at open().  Then during file's write/read
additional checks might be performed. BPF has such functionality already.
Different map_creates have capability checks while map_lookup has:
map_get_sys_perms(map, f) & FMODE_CAN_READ.
In other words it's enough to gate FD-receiving parts of bpf
with unprivileged_bpf_disabled sysctl.
The rest is handled by availability of FD and access to files in bpffs."

So key fd creation syscall commands BPF_PROG_LOAD and BPF_MAP_CREATE
are blocked with unprivileged BPF disabled and no CAP_BPF.

And as Alexei notes, map creation with unprivileged BPF disabled off
blocks creation of maps aside from array, hash and ringbuf maps.

Programs responsible for loading and attaching the BPF program
can still control access to its pinned representation by restricting
permissions on the pin path, as with normal files.

Fixes: 1be7f75d1668 ("bpf: enable non-root eBPF programs")
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 kernel/bpf/syscall.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 72e53489165d..2b69306d3c6e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -4863,9 +4863,21 @@ static int bpf_prog_bind_map(union bpf_attr *attr)
 static int __sys_bpf(int cmd, bpfptr_t uattr, unsigned int size)
 {
 	union bpf_attr attr;
+	bool capable;
 	int err;
 
-	if (sysctl_unprivileged_bpf_disabled && !bpf_capable())
+	capable = bpf_capable() || !sysctl_unprivileged_bpf_disabled;
+
+	/* Intent here is for unprivileged_bpf_disabled to block key object
+	 * creation commands for unprivileged users; other actions depend
+	 * of fd availability and access to bpffs, so are dependent on
+	 * object creation success.  Capabilities are later verified for
+	 * operations such as load and map create, so even with unprivileged
+	 * BPF disabled, capability checks are still carried out for these
+	 * and other operations.
+	 */
+	if (!capable &&
+	    (cmd == BPF_MAP_CREATE || cmd == BPF_PROG_LOAD))
 		return -EPERM;
 
 	err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
-- 
2.27.0

Re: [PATCH v2 bpf-next 1/2] bpf: refine kernel.unpriviliged_bpf_disabled behaviour

[Yonghong Song]

On 5/17/22 4:59 AM, Alan Maguire wrote:
> With unprivileged BPF disabled, all cmds associated with the BPF syscall
> are blocked to users without CAP_BPF/CAP_SYS_ADMIN.  However there are
> use cases where we may wish to allow interactions with BPF programs
> without being able to load and attach them.  So for example, a process
> with required capabilities loads/attaches a BPF program, and a process
> with less capabilities interacts with it; retrieving perf/ring buffer
> events, modifying map-specified config etc.  With all BPF syscall
> commands blocked as a result of unprivileged BPF being disabled,
> this mode of interaction becomes impossible for processes without
> CAP_BPF.
> 
> As Alexei notes
> 
> "The bpf ACL model is the same as traditional file's ACL.
> The creds and ACLs are checked at open().  Then during file's write/read
> additional checks might be performed. BPF has such functionality already.
> Different map_creates have capability checks while map_lookup has:
> map_get_sys_perms(map, f) & FMODE_CAN_READ.
> In other words it's enough to gate FD-receiving parts of bpf
> with unprivileged_bpf_disabled sysctl.
> The rest is handled by availability of FD and access to files in bpffs."
> 
> So key fd creation syscall commands BPF_PROG_LOAD and BPF_MAP_CREATE
> are blocked with unprivileged BPF disabled and no CAP_BPF.
> 
> And as Alexei notes, map creation with unprivileged BPF disabled off
> blocks creation of maps aside from array, hash and ringbuf maps.
> 
> Programs responsible for loading and attaching the BPF program
> can still control access to its pinned representation by restricting
> permissions on the pin path, as with normal files.
> 
> Fixes: 1be7f75d1668 ("bpf: enable non-root eBPF programs")
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

Acked-by: Yonghong Song <yhs@fb.com>

[PATCH v2 bpf-next 2/2] selftests/bpf: add tests verifying unprivileged bpf disabled behaviour

[Alan Maguire]

tests load/attach bpf prog with maps, perfbuf and ringbuf, pinning
them.  Then effective caps are dropped and we verify we can

- pick up the pin
- create ringbuf/perfbuf
- get ringbuf/perfbuf events, carry out map update, lookup and delete
- create a link

Negative testing also ensures

- BPF prog load fails
- BPF map create fails
- get fd by id fails
- get next id fails
- query fails
- BTF load fails

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 .../bpf/prog_tests/unpriv_bpf_disabled.c      | 308 ++++++++++++++++++
 .../bpf/progs/test_unpriv_bpf_disabled.c      |  83 +++++
 2 files changed, 391 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c

diff --git a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
new file mode 100644
index 000000000000..7c58c4f7ecc7
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022, Oracle and/or its affiliates. */
+
+#include <test_progs.h>
+#include <bpf/btf.h>
+
+#include "test_unpriv_bpf_disabled.skel.h"
+
+#include "cap_helpers.h"
+
+#define ADMIN_CAPS (1ULL << CAP_SYS_ADMIN |	\
+		    1ULL << CAP_NET_ADMIN |	\
+		    1ULL << CAP_PERFMON |	\
+		    1ULL << CAP_BPF)
+
+#define PINPATH		"/sys/fs/bpf/unpriv_bpf_disabled_"
+
+struct test_unpriv_bpf_disabled *skel;
+__u32 prog_id;
+int prog_fd;
+int perf_fd;
+char *map_paths[7] =	{ PINPATH "array",
+			  PINPATH "percpu_array",
+			  PINPATH "hash",
+			  PINPATH "percpu_hash",
+			  PINPATH "perfbuf",
+			  PINPATH "ringbuf",
+			  PINPATH "prog_array" };
+int map_fds[7];
+
+static __u32 got_perfbuf_val;
+static __u32 got_ringbuf_val;
+
+static int process_ringbuf(void *ctx, void *data, size_t len)
+{
+	if (len == sizeof(__u32))
+		got_ringbuf_val = *(__u32 *)data;
+	return 0;
+}
+
+static void process_perfbuf(void *ctx, int cpu, void *data, __u32 len)
+{
+	if (len == sizeof(__u32))
+		got_perfbuf_val = *(__u32 *)data;
+}
+
+static int sysctl_set(const char *sysctl_path, char *old_val, const char *new_val)
+{
+	int ret = 0;
+	FILE *fp;
+
+	fp = fopen(sysctl_path, "r+");
+	if (!fp)
+		return -errno;
+	if (old_val && fscanf(fp, "%s", old_val) <= 0) {
+		ret = -ENOENT;
+	} else if (!old_val || strcmp(old_val, new_val) != 0) {
+		fseek(fp, 0, SEEK_SET);
+		if (fprintf(fp, "%s", new_val) < 0)
+			ret = -errno;
+	}
+	fclose(fp);
+
+	return ret;
+}
+
+static void test_unpriv_bpf_disabled_positive(void)
+{
+	struct perf_buffer *perfbuf = NULL;
+	struct ring_buffer *ringbuf = NULL;
+	int i, nr_cpus, link_fd = -1;
+
+	nr_cpus = bpf_num_possible_cpus();
+
+	skel->bss->perfbuf_val = 1;
+	skel->bss->ringbuf_val = 2;
+
+	/* Positive tests for unprivileged BPF disabled. Verify we can
+	 * - retrieve and interact with pinned maps;
+	 * - set up and interact with perf buffer;
+	 * - set up and interact with ring buffer;
+	 * - create a link
+	 */
+	perfbuf = perf_buffer__new(bpf_map__fd(skel->maps.perfbuf), 8, process_perfbuf, NULL, NULL,
+				   NULL);
+	if (!ASSERT_OK_PTR(perfbuf, "perf_buffer__new"))
+		goto cleanup;
+
+	ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.ringbuf), process_ringbuf, NULL, NULL);
+	if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
+		goto cleanup;
+
+	/* trigger & validate perf event, ringbuf output */
+	usleep(1);
+
+	ASSERT_GT(perf_buffer__poll(perfbuf, 100), -1, "perf_buffer__poll");
+
+	ASSERT_EQ(got_perfbuf_val, skel->bss->perfbuf_val, "check_perfbuf_val");
+
+	ASSERT_EQ(ring_buffer__consume(ringbuf), 1, "ring_buffer__consume");
+
+	ASSERT_EQ(got_ringbuf_val, skel->bss->ringbuf_val, "check_ringbuf_val");
+
+	for (i = 0; i < ARRAY_SIZE(map_fds); i++) {
+		map_fds[i] = bpf_obj_get(map_paths[i]);
+		if (!ASSERT_GT(map_fds[i], -1, "obj_get"))
+			goto cleanup;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(map_fds); i++) {
+		bool prog_array = strstr(map_paths[i], "prog_array") != NULL;
+		bool array = strstr(map_paths[i], "array") != NULL;
+		bool buf = strstr(map_paths[i], "buf") != NULL;
+		__u32 key = 0, vals[nr_cpus], lookup_vals[nr_cpus];
+		__u32 expected_val = 1;
+		int j;
+
+		/* skip ringbuf, perfbuf */
+		if (buf)
+			continue;
+
+		for (j = 0; j < nr_cpus; j++)
+			vals[j] = expected_val;
+
+		if (prog_array) {
+			/* need valid prog array value */
+			vals[0] = prog_fd;
+			/* prog array lookup returns prog id, not fd */
+			expected_val = prog_id;
+		}
+		ASSERT_OK(bpf_map_update_elem(map_fds[i], &key, vals, 0), "map_update_elem");
+		ASSERT_OK(bpf_map_lookup_elem(map_fds[i], &key, &lookup_vals), "map_lookup_elem");
+		ASSERT_EQ(lookup_vals[0], expected_val, "map_lookup_elem_values");
+		if (!array)
+			ASSERT_OK(bpf_map_delete_elem(map_fds[i], &key), "map_delete_elem");
+	}
+
+	link_fd = bpf_link_create(bpf_program__fd(skel->progs.handle_perf_event), perf_fd,
+				  BPF_PERF_EVENT, NULL);
+	ASSERT_GT(link_fd, 0, "link_create");
+
+cleanup:
+	if (link_fd)
+		close(link_fd);
+	if (perfbuf)
+		perf_buffer__free(perfbuf);
+	if (ringbuf)
+		ring_buffer__free(ringbuf);
+}
+
+static void test_unpriv_bpf_disabled_negative(void)
+{
+	const struct bpf_insn prog_insns[] = {
+		BPF_MOV64_IMM(BPF_REG_0, 0),
+		BPF_EXIT_INSN(),
+	};
+	const size_t prog_insn_cnt = sizeof(prog_insns) / sizeof(struct bpf_insn);
+	LIBBPF_OPTS(bpf_prog_load_opts, load_opts);
+	struct bpf_map_info map_info = {};
+	__u32 map_info_len = sizeof(map_info);
+	struct bpf_link_info link_info = {};
+	__u32 link_info_len = sizeof(link_info);
+	struct btf *btf = NULL;
+	__u32 attach_flags = 0;
+	__u32 prog_ids[3] = {};
+	__u32 prog_cnt = 3;
+	__u32 next;
+	int i;
+
+	/* Negative tests for unprivileged BPF disabled.  Verify we cannot
+	 * - load BPF programs;
+	 * - create BPF maps;
+	 * - get a prog/map/link fd by id;
+	 * - get next prog/map/link id
+	 * - query prog
+	 * - BTF load
+	 */
+	ASSERT_EQ(bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, "simple_prog", "GPL",
+				prog_insns, prog_insn_cnt, &load_opts),
+		  -EPERM, "prog_load_fails");
+
+	for (i = BPF_MAP_TYPE_HASH; i <= BPF_MAP_TYPE_BLOOM_FILTER; i++)
+		ASSERT_EQ(bpf_map_create(i, NULL, sizeof(int), sizeof(int), 1, NULL),
+			  -EPERM, "map_create_fails");
+
+	ASSERT_EQ(bpf_prog_get_fd_by_id(prog_id), -EPERM, "prog_get_fd_by_id_fails");
+	ASSERT_EQ(bpf_prog_get_next_id(prog_id, &next), -EPERM, "prog_get_next_id_fails");
+	ASSERT_EQ(bpf_prog_get_next_id(0, &next), -EPERM, "prog_get_next_id_fails");
+
+	if (ASSERT_OK(bpf_obj_get_info_by_fd(map_fds[0], &map_info, &map_info_len),
+		      "obj_get_info_by_fd")) {
+		ASSERT_EQ(bpf_map_get_fd_by_id(map_info.id), -EPERM, "map_get_fd_by_id_fails");
+		ASSERT_EQ(bpf_map_get_next_id(map_info.id, &next), -EPERM,
+			  "map_get_next_id_fails");
+	}
+	ASSERT_EQ(bpf_map_get_next_id(0, &next), -EPERM, "map_get_next_id_fails");
+
+	if (ASSERT_OK(bpf_obj_get_info_by_fd(bpf_link__fd(skel->links.sys_nanosleep_enter),
+					     &link_info, &link_info_len),
+		      "obj_get_info_by_fd")) {
+		ASSERT_EQ(bpf_link_get_fd_by_id(link_info.id), -EPERM, "link_get_fd_by_id_fails");
+		ASSERT_EQ(bpf_link_get_next_id(link_info.id, &next), -EPERM,
+			  "link_get_next_id_fails");
+	}
+	ASSERT_EQ(bpf_link_get_next_id(0, &next), -EPERM, "link_get_next_id_fails");
+
+	ASSERT_EQ(bpf_prog_query(prog_fd, BPF_PROG_TYPE_TRACING, 0, &attach_flags, prog_ids,
+				 &prog_cnt), -EPERM, "prog_query_fails");
+
+	btf = btf__new_empty();
+	if (ASSERT_OK_PTR(btf, "empty_btf") &&
+	    ASSERT_GT(btf__add_int(btf, "int", 4, 0), 0, "unpriv_int_type")) {
+		const void *raw_btf_data;
+		__u32 raw_btf_size;
+
+		raw_btf_data = btf__raw_data(btf, &raw_btf_size);
+		if (ASSERT_OK_PTR(raw_btf_data, "raw_btf_data_good"))
+			ASSERT_EQ(bpf_btf_load(raw_btf_data, raw_btf_size, NULL), -EPERM,
+				  "bpf_btf_load_fails");
+	}
+	btf__free(btf);
+}
+
+void test_unpriv_bpf_disabled(void)
+{
+	char unprivileged_bpf_disabled_orig[32] = {};
+	char perf_event_paranoid_orig[32] = {};
+	struct bpf_prog_info prog_info = {};
+	__u32 prog_info_len = sizeof(prog_info);
+	struct perf_event_attr attr = {};
+	__u64 save_caps = 0;
+	int i, ret;
+
+	skel = test_unpriv_bpf_disabled__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	skel->bss->test_pid = getpid();
+
+	map_fds[0] = bpf_map__fd(skel->maps.array);
+	map_fds[1] = bpf_map__fd(skel->maps.percpu_array);
+	map_fds[2] = bpf_map__fd(skel->maps.hash);
+	map_fds[3] = bpf_map__fd(skel->maps.percpu_hash);
+	map_fds[4] = bpf_map__fd(skel->maps.perfbuf);
+	map_fds[5] = bpf_map__fd(skel->maps.ringbuf);
+	map_fds[6] = bpf_map__fd(skel->maps.prog_array);
+
+	for (i = 0; i < ARRAY_SIZE(map_fds); i++)
+		ASSERT_OK(bpf_obj_pin(map_fds[i], map_paths[i]), "pin map_fd");
+
+	/* allow user without caps to use perf events */
+	if (!ASSERT_OK(sysctl_set("/proc/sys/kernel/perf_event_paranoid", perf_event_paranoid_orig,
+				  "-1"),
+		       "set_perf_event_paranoid"))
+		goto cleanup;
+	/* ensure unprivileged bpf disabled is set */
+	ret = sysctl_set("/proc/sys/kernel/unprivileged_bpf_disabled",
+			 unprivileged_bpf_disabled_orig, "2");
+	if (ret == -EPERM) {
+		/* if unprivileged_bpf_disabled=1, we get -EPERM back; that's okay. */
+		if (!ASSERT_OK(strcmp(unprivileged_bpf_disabled_orig, "1"),
+			       "unpriviliged_bpf_disabled_on"))
+			goto cleanup;
+	} else {
+		if (!ASSERT_OK(ret, "set unpriviliged_bpf_disabled"))
+			goto cleanup;
+	}
+
+	prog_fd = bpf_program__fd(skel->progs.sys_nanosleep_enter);
+	ASSERT_OK(bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len),
+		  "obj_get_info_by_fd");
+	prog_id = prog_info.id;
+	ASSERT_GT(prog_id, 0, "valid_prog_id");
+
+	attr.size = sizeof(attr);
+	attr.type = PERF_TYPE_SOFTWARE;
+	attr.config = PERF_COUNT_SW_CPU_CLOCK;
+	attr.freq = 1;
+	attr.sample_freq = 1000;
+	perf_fd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, PERF_FLAG_FD_CLOEXEC);
+	if (!ASSERT_GE(perf_fd, 0, "perf_fd"))
+		goto cleanup;
+
+	if (!ASSERT_OK(test_unpriv_bpf_disabled__attach(skel), "skel_attach"))
+		goto cleanup;
+
+	if (!ASSERT_OK(cap_disable_effective(ADMIN_CAPS, &save_caps), "disable caps"))
+		goto cleanup;
+
+	if (test__start_subtest("unpriv_bpf_disabled_positive"))
+		test_unpriv_bpf_disabled_positive();
+
+	if (test__start_subtest("unpriv_bpf_disabled_negative"))
+		test_unpriv_bpf_disabled_negative();
+
+cleanup:
+	close(perf_fd);
+	if (save_caps)
+		cap_enable_effective(save_caps, NULL);
+	if (strlen(perf_event_paranoid_orig) > 0)
+		sysctl_set("/proc/sys/kernel/perf_event_paranoid", NULL, perf_event_paranoid_orig);
+	if (strlen(unprivileged_bpf_disabled_orig) > 0)
+		sysctl_set("/proc/sys/kernel/unprivileged_bpf_disabled", NULL,
+			   unprivileged_bpf_disabled_orig);
+	for (i = 0; i < ARRAY_SIZE(map_paths); i++)
+		unlink(map_paths[i]);
+	test_unpriv_bpf_disabled__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c b/tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c
new file mode 100644
index 000000000000..fc423e43a3cd
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022, Oracle and/or its affiliates. */
+
+#include "vmlinux.h"
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+__u32 perfbuf_val = 0;
+__u32 ringbuf_val = 0;
+
+int test_pid;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u32);
+} array SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u32);
+} percpu_array SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u32);
+} hash SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_HASH);
+	__uint(max_entries, 1);
+	__type(key, __u32);
+	__type(value, __u32);
+} percpu_hash SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
+	__type(key, __u32);
+	__type(value, __u32);
+} perfbuf SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_RINGBUF);
+	__uint(max_entries, 1 << 12);
+} ringbuf SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
+	__uint(max_entries, 1);
+	__uint(key_size, sizeof(__u32));
+	__uint(value_size, sizeof(__u32));
+} prog_array SEC(".maps");
+
+SEC("fentry/" SYS_PREFIX "sys_nanosleep")
+int sys_nanosleep_enter(void *ctx)
+{
+	int cur_pid;
+
+	cur_pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (cur_pid != test_pid)
+		return 0;
+
+	bpf_perf_event_output(ctx, &perfbuf, BPF_F_CURRENT_CPU, &perfbuf_val, sizeof(perfbuf_val));
+	bpf_ringbuf_output(&ringbuf, &ringbuf_val, sizeof(ringbuf_val), 0);
+
+	return 0;
+}
+
+SEC("perf_event")
+int handle_perf_event(void *ctx)
+{
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.27.0

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests verifying unprivileged bpf disabled behaviour

[Alexei Starovoitov]

On Tue, May 17, 2022 at 5:00 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> tests load/attach bpf prog with maps, perfbuf and ringbuf, pinning
> them.  Then effective caps are dropped and we verify we can
>
> - pick up the pin
> - create ringbuf/perfbuf
> - get ringbuf/perfbuf events, carry out map update, lookup and delete
> - create a link
>
> Negative testing also ensures
>
> - BPF prog load fails
> - BPF map create fails
> - get fd by id fails
> - get next id fails
> - query fails
> - BTF load fails
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

It fails with clang build.
See BPF CI.
prog_tests/unpriv_bpf_disabled.c:207:36: error: implicit conversion
from enumeration type 'enum bpf_prog_type' to different enumeration
type 'enum bpf_attach_type' [-Werror,-Wenum-conversion]
ASSERT_EQ(bpf_prog_query(prog_fd, BPF_PROG_TYPE_TRACING, 0,
&attach_flags, prog_ids,

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests verifying unprivileged bpf disabled behaviour

[Yonghong Song]

On 5/17/22 4:59 AM, Alan Maguire wrote:
> tests load/attach bpf prog with maps, perfbuf and ringbuf, pinning
> them.  Then effective caps are dropped and we verify we can
> 
> - pick up the pin
> - create ringbuf/perfbuf
> - get ringbuf/perfbuf events, carry out map update, lookup and delete
> - create a link
> 
> Negative testing also ensures
> 
> - BPF prog load fails
> - BPF map create fails
> - get fd by id fails
> - get next id fails
> - query fails
> - BTF load fails
> 
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>

LGTM except a few minor nits below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../bpf/prog_tests/unpriv_bpf_disabled.c      | 308 ++++++++++++++++++
>   .../bpf/progs/test_unpriv_bpf_disabled.c      |  83 +++++
>   2 files changed, 391 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
>   create mode 100644 tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
> new file mode 100644
> index 000000000000..7c58c4f7ecc7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022, Oracle and/or its affiliates. */
> +
> +#include <test_progs.h>
> +#include <bpf/btf.h>
> +
> +#include "test_unpriv_bpf_disabled.skel.h"
> +
> +#include "cap_helpers.h"
> +
> +#define ADMIN_CAPS (1ULL << CAP_SYS_ADMIN |	\
> +		    1ULL << CAP_NET_ADMIN |	\
> +		    1ULL << CAP_PERFMON |	\
> +		    1ULL << CAP_BPF)

Not sure whether we could simply disable all capabilities
for this particular test since we know what capabilities
are need for bpf subsystem.

> +
> +#define PINPATH		"/sys/fs/bpf/unpriv_bpf_disabled_"
> +
> +struct test_unpriv_bpf_disabled *skel;
> +__u32 prog_id;
> +int prog_fd;
> +int perf_fd;

int prog_fd, perf_fd?

> +char *map_paths[7] =	{ PINPATH "array",
> +			  PINPATH "percpu_array",
> +			  PINPATH "hash",
> +			  PINPATH "percpu_hash",
> +			  PINPATH "perfbuf",
> +			  PINPATH "ringbuf",
> +			  PINPATH "prog_array" };

define a macro for '7' and used below as well?

> +int map_fds[7];
> +
> +static __u32 got_perfbuf_val;
> +static __u32 got_ringbuf_val;
> +
> +static int process_ringbuf(void *ctx, void *data, size_t len)
> +{
> +	if (len == sizeof(__u32))

ASSERT if len != sizeof(__u32)?

> +		got_ringbuf_val = *(__u32 *)data;
> +	return 0;
> +}
> +
> +static void process_perfbuf(void *ctx, int cpu, void *data, __u32 len)
> +{
> +	if (len == sizeof(__u32))

ASSERT if len != sizeof(__u32)?

> +		got_perfbuf_val = *(__u32 *)data;
> +}
> +
> +static int sysctl_set(const char *sysctl_path, char *old_val, const char *new_val)
> +{
> +	int ret = 0;
> +	FILE *fp;
> +
> +	fp = fopen(sysctl_path, "r+");
> +	if (!fp)
> +		return -errno;
> +	if (old_val && fscanf(fp, "%s", old_val) <= 0) {
> +		ret = -ENOENT;
> +	} else if (!old_val || strcmp(old_val, new_val) != 0) {
> +		fseek(fp, 0, SEEK_SET);
> +		if (fprintf(fp, "%s", new_val) < 0)
> +			ret = -errno;
> +	}
> +	fclose(fp);
> +
> +	return ret;
> +}
> +
> +static void test_unpriv_bpf_disabled_positive(void)
> +{
> +	struct perf_buffer *perfbuf = NULL;
> +	struct ring_buffer *ringbuf = NULL;
> +	int i, nr_cpus, link_fd = -1;
> +
> +	nr_cpus = bpf_num_possible_cpus();
> +
> +	skel->bss->perfbuf_val = 1;
> +	skel->bss->ringbuf_val = 2;
> +
> +	/* Positive tests for unprivileged BPF disabled. Verify we can
> +	 * - retrieve and interact with pinned maps;
> +	 * - set up and interact with perf buffer;
> +	 * - set up and interact with ring buffer;
> +	 * - create a link
> +	 */
> +	perfbuf = perf_buffer__new(bpf_map__fd(skel->maps.perfbuf), 8, process_perfbuf, NULL, NULL,
> +				   NULL);
> +	if (!ASSERT_OK_PTR(perfbuf, "perf_buffer__new"))
> +		goto cleanup;
> +
> +	ringbuf = ring_buffer__new(bpf_map__fd(skel->maps.ringbuf), process_ringbuf, NULL, NULL);
> +	if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new"))
> +		goto cleanup;
> +
> +	/* trigger & validate perf event, ringbuf output */
> +	usleep(1);
> +
> +	ASSERT_GT(perf_buffer__poll(perfbuf, 100), -1, "perf_buffer__poll");
> +
> +	ASSERT_EQ(got_perfbuf_val, skel->bss->perfbuf_val, "check_perfbuf_val");
> +
> +	ASSERT_EQ(ring_buffer__consume(ringbuf), 1, "ring_buffer__consume");
> +
> +	ASSERT_EQ(got_ringbuf_val, skel->bss->ringbuf_val, "check_ringbuf_val");

You can remove empty lines between above ASSERT_* statements.

> +
> +	for (i = 0; i < ARRAY_SIZE(map_fds); i++) {
> +		map_fds[i] = bpf_obj_get(map_paths[i]);
> +		if (!ASSERT_GT(map_fds[i], -1, "obj_get"))
> +			goto cleanup;
> +	}
> +
[...]

Re: [PATCH v2 bpf-next 2/2] selftests/bpf: add tests verifying unprivileged bpf disabled behaviour

[Andrii Nakryiko]

On Tue, May 17, 2022 at 5:00 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> tests load/attach bpf prog with maps, perfbuf and ringbuf, pinning
> them.  Then effective caps are dropped and we verify we can
>
> - pick up the pin
> - create ringbuf/perfbuf
> - get ringbuf/perfbuf events, carry out map update, lookup and delete
> - create a link
>
> Negative testing also ensures
>
> - BPF prog load fails
> - BPF map create fails
> - get fd by id fails
> - get next id fails
> - query fails
> - BTF load fails
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
>  .../bpf/prog_tests/unpriv_bpf_disabled.c      | 308 ++++++++++++++++++
>  .../bpf/progs/test_unpriv_bpf_disabled.c      |  83 +++++
>  2 files changed, 391 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_unpriv_bpf_disabled.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
> new file mode 100644
> index 000000000000..7c58c4f7ecc7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2022, Oracle and/or its affiliates. */
> +
> +#include <test_progs.h>
> +#include <bpf/btf.h>
> +
> +#include "test_unpriv_bpf_disabled.skel.h"
> +
> +#include "cap_helpers.h"
> +
> +#define ADMIN_CAPS (1ULL << CAP_SYS_ADMIN |    \
> +                   1ULL << CAP_NET_ADMIN |     \
> +                   1ULL << CAP_PERFMON |       \
> +                   1ULL << CAP_BPF)
> +
> +#define PINPATH                "/sys/fs/bpf/unpriv_bpf_disabled_"
> +
> +struct test_unpriv_bpf_disabled *skel;
> +__u32 prog_id;
> +int prog_fd;
> +int perf_fd;
> +char *map_paths[7] =   { PINPATH "array",
> +                         PINPATH "percpu_array",
> +                         PINPATH "hash",
> +                         PINPATH "percpu_hash",
> +                         PINPATH "perfbuf",
> +                         PINPATH "ringbuf",
> +                         PINPATH "prog_array" };
> +int map_fds[7];

just very briefly skimming, all these variables should be static

but at least for skel why not passing it as input argument to
respective subtest functions?

> +
> +static __u32 got_perfbuf_val;
> +static __u32 got_ringbuf_val;
> +

[...]