[RFC bpf-next 0/2] bpf: allow unprivileged map access to some map types

[RFC bpf-next 0/2] bpf: allow unprivileged map access to some map types

[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 use cases
prior to having unpriviliged_bpf_disabled set to >= 1 however,
it made 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, after it has been loaded, interacts with it via
pinned maps.

Such a split model is not possible with unprivileged BPF disabled,
since even those activities such as map interactions, retrieving
map information from pinned paths etc are blocked to
unprivileged users.  However, granting CAP_BPF to such unprivileged
users is quite a big hammer, allowing them to do pretty much
anything with BPF.

This very rough RFC explores the idea - with
CONFIG_BPF_UNPRIV_MAP_ACCESS=y - of allowing unprivileged processes
to retrieve and work with a restricted set of BPF maps.  See
patch 1 for the restrictions on BPF cmd and map type. Note that
permission checks on maps are still enforced, so it's still
possible to prevent unwanted interference by unprivileged users
by pinning a map and setting permissions to prevent access.
CONFIG_BPF_UNPRIV_MAP_ACCESS defaults to n, preserving current
behaviour of blocking all BPF syscall commands.

Discussion on the bpf mailing list already alluded to this idea [1],
though it's possible I misinterpreted it.

There are other ways to achieve this goal I suspect; for example
a BPF LSM program attached to security_bpf() could weed out the
disallowed commands, so setting unprivileged_bpf_disabled=0 + BPF
LSM could support a wider range of policies (not sure if we
could easily determine map types in that case though).
However, as a starting point I just wanted to see if others see
this as an issue, and if so how best to solve it in the general
case. Thanks!

[1] https://lore.kernel.org/bpf/CAADnVQLTBhCTAx1a_nev7CgMZxv1Bb7ecz1AFRin8tHmjPREJA@mail.gmail.com/

Alan Maguire (2):
  bpf: with CONFIG_BPF_UNPRIV_MAP_ACCESS=y, allow unprivileged access to
    BPF maps
  selftests/bpf: add tests verifying unpriv bpf map access

 kernel/bpf/Kconfig                            |  15 ++
 kernel/bpf/syscall.c                          |  57 ++++-
 tools/testing/selftests/bpf/config            |   1 +
 .../bpf/prog_tests/unpriv_map_access.c        | 194 ++++++++++++++++++
 .../bpf/progs/test_unpriv_map_access.c        |  81 ++++++++
 5 files changed, 347 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/unpriv_map_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_unpriv_map_access.c

-- 
2.27.0

[RFC bpf-next 1/2] bpf: with CONFIG_BPF_UNPRIV_MAP_ACCESS=y, allow unprivileged access to BPF maps

[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.

Here we propose allowing BPF_OBJ_GET (to retrieve pinned map/prog) and
BPF_MAP_* commands to work, even in cases where unprivileged BPF is
disabled and appropriate capabilities are not present.  This mode of
operation is not enabled by default; it depends on setting
CONFIG_BPF_UNPRIV_MAP_ACCESS=y (it defaults to n).

Note that the program responsible for loading and attaching the BPF program
can still control access to its pinned representation by restricting
permissions on the pin path.

For map-related BPF syscalls under CONFIG_BPF_UNPRIV_MAP_ACCESS=y,
map access is restricted to the following map types:

BPF_MAP_TYPE_ARRAY
BPF_MAP_TYPE_HASH
BPF_MAP_TYPE_PERF_EVENT_ARRAY
BPF_MAP_TYPE_PERCPU_ARRAY
BPF_MAP_TYPE_PERCPU_HASH
BPF_MAP_TYPE_RINGBUF

The set of unprivileged BPF syscall commands that are permitted for the
above map types with CONFIG_BPF_UNPRIV_MAP_ACCESS=y is

BPF_MAP_LOOKUP_ELEM
BPF_MAP_UPDATE_ELEM
BPF_MAP_DELETE_ELEM
BPF_MAP_NEXT_KEY

..and the following unprivileged BPF syscall commands are permitted
in order to allow map retrieval:

BPF_OBJ_GET
BPF_OBJ_GET_INFO_BY_FD

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 kernel/bpf/Kconfig   | 15 ++++++++++++
 kernel/bpf/syscall.c | 57 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/Kconfig b/kernel/bpf/Kconfig
index d56ee177d5f8..30e6f559ad08 100644
--- a/kernel/bpf/Kconfig
+++ b/kernel/bpf/Kconfig
@@ -84,6 +84,21 @@ config BPF_UNPRIV_DEFAULT_OFF
 
 	  If you are unsure how to answer this question, answer Y.
 
+config BPF_UNPRIV_MAP_ACCESS
+	bool "Allow unprivileged access to BPF map-related actions"
+	default n
+	depends on BPF_SYSCALL
+	help
+	   Allow unprivileged access to retrieve pinned objects, lookup,
+	   update and delete map elements.  Only specific BPF map types
+	   are permitted - (per-cpu) array maps, (per-cpu) hash maps,
+	   perf event and ring buffer maps.
+
+	   This allows limited use of the BPF syscall for unprivileged
+	   users; note however that this does not include loading or
+	   attaching BPF programs.  Pinned maps can also specify
+	   pin path permissions to prevent unwanted access.
+
 source "kernel/bpf/preload/Kconfig"
 
 config BPF_LSM
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 72e53489165d..951491836bbb 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1278,6 +1278,22 @@ static void *___bpf_copy_key(bpfptr_t ukey, u64 key_size)
 	return NULL;
 }
 
+static bool map_type_prevent_unprivileged_access(struct bpf_map *map)
+{
+#ifdef CONFIG_BPF_UNPRIV_MAP_ACCESS
+	return sysctl_unprivileged_bpf_disabled && !bpf_capable() &&
+	       map->map_type != BPF_MAP_TYPE_ARRAY &&
+	       map->map_type != BPF_MAP_TYPE_HASH &&
+	       map->map_type != BPF_MAP_TYPE_PERF_EVENT_ARRAY &&
+	       map->map_type != BPF_MAP_TYPE_PERCPU_ARRAY &&
+	       map->map_type != BPF_MAP_TYPE_PERCPU_HASH &&
+	       map->map_type != BPF_MAP_TYPE_RINGBUF;
+#else
+	/* earlier checks prevent unprivileged access */
+	return false;
+#endif
+}
+
 /* last field in 'union bpf_attr' used by this command */
 #define BPF_MAP_LOOKUP_ELEM_LAST_FIELD flags
 
@@ -1313,6 +1329,11 @@ static int map_lookup_elem(union bpf_attr *attr)
 		goto err_put;
 	}
 
+	if (map_type_prevent_unprivileged_access(map)) {
+		err = -EPERM;
+		goto err_put;
+	}
+
 	key = __bpf_copy_key(ukey, map->key_size);
 	if (IS_ERR(key)) {
 		err = PTR_ERR(key);
@@ -1386,6 +1407,11 @@ static int map_update_elem(union bpf_attr *attr, bpfptr_t uattr)
 		goto err_put;
 	}
 
+	if (map_type_prevent_unprivileged_access(map)) {
+		err = -EPERM;
+		goto err_put;
+	}
+
 	key = ___bpf_copy_key(ukey, map->key_size);
 	if (IS_ERR(key)) {
 		err = PTR_ERR(key);
@@ -1439,6 +1465,11 @@ static int map_delete_elem(union bpf_attr *attr)
 		goto err_put;
 	}
 
+	if (map_type_prevent_unprivileged_access(map)) {
+		err = -EPERM;
+		goto err_put;
+	}
+
 	key = __bpf_copy_key(ukey, map->key_size);
 	if (IS_ERR(key)) {
 		err = PTR_ERR(key);
@@ -1494,6 +1525,11 @@ static int map_get_next_key(union bpf_attr *attr)
 		goto err_put;
 	}
 
+	if (map_type_prevent_unprivileged_access(map)) {
+		err = -EPERM;
+		goto err_put;
+	}
+
 	if (ukey) {
 		key = __bpf_copy_key(ukey, map->key_size);
 		if (IS_ERR(key)) {
@@ -4863,10 +4899,29 @@ 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;
+
+#ifdef CONFIG_BPF_UNPRIV_MAP_ACCESS
+	/* A subset of cmds are allowed to unprivileged users, principally to allow
+	 * them to interact with pinned BPF maps to retrieve events, update map
+	 * values etc.  The pinning program can adjust pin path permissions
+	 * to prevent unwanted access by unprivileged users.
+	 */
+	if (!capable &&
+	    cmd != BPF_MAP_LOOKUP_ELEM &&
+	    cmd != BPF_MAP_UPDATE_ELEM &&
+	    cmd != BPF_MAP_DELETE_ELEM &&
+	    cmd != BPF_MAP_GET_NEXT_KEY &&
+	    cmd != BPF_OBJ_GET &&
+	    cmd != BPF_OBJ_GET_INFO_BY_FD)
 		return -EPERM;
+#else
+	if (!capable)
+		return -EPERM;
+#endif
 
 	err = bpf_check_uarg_tail_zero(uattr, sizeof(attr), size);
 	if (err)
-- 
2.27.0

[RFC bpf-next 2/2] selftests/bpf: add tests verifying unpriv bpf map access

[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

Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
 tools/testing/selftests/bpf/config            |   1 +
 .../bpf/prog_tests/unpriv_map_access.c        | 194 ++++++++++++++++++
 .../bpf/progs/test_unpriv_map_access.c        |  81 ++++++++
 3 files changed, 276 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/unpriv_map_access.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_unpriv_map_access.c

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index 763db63a3890..f9570f93ba29 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -36,6 +36,7 @@ CONFIG_MPLS_ROUTING=m
 CONFIG_MPLS_IPTUNNEL=m
 CONFIG_IPV6_SIT=m
 CONFIG_BPF_JIT=y
+CONFIG_BPF_UNPRIV_MAP_ACCESS=y
 CONFIG_BPF_LSM=y
 CONFIG_SECURITY=y
 CONFIG_RC_CORE=y
diff --git a/tools/testing/selftests/bpf/prog_tests/unpriv_map_access.c b/tools/testing/selftests/bpf/prog_tests/unpriv_map_access.c
new file mode 100644
index 000000000000..cd64a38ee075
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/unpriv_map_access.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022, Oracle and/or its affiliates. */
+
+#include <test_progs.h>
+#include "test_unpriv_map_access.skel.h"
+
+#include "cap_helpers.h"
+
+/* need CAP_BPF, CAP_NET_ADMIN, CAP_PERFMON to load progs */
+#define ADMIN_CAPS (1ULL << CAP_SYS_ADMIN |	\
+		    1ULL << CAP_PERFMON |	\
+		    1ULL << CAP_BPF)
+
+#define PINPATH		"/sys/fs/bpf/unpriv_map_access_"
+
+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;
+}
+
+void test_unpriv_map_access(void)
+{
+	struct test_unpriv_map_access *skel;
+	struct perf_buffer *perfbuf = NULL;
+	struct ring_buffer *ringbuf = NULL;
+	__u64 save_caps = 0;
+	int i, ret, nr_cpus, map_fds[7];
+	char *map_paths[7] = { PINPATH "array",
+			       PINPATH "percpu_array",
+			       PINPATH "hash",
+			       PINPATH "percpu_hash",
+			       PINPATH "perfbuf",
+			       PINPATH "ringbuf",
+			       PINPATH "prog_array" };
+	char unprivileged_bpf_disabled_orig[32] = {};
+	char perf_event_paranoid_orig[32] = {};
+
+	nr_cpus = bpf_num_possible_cpus();
+
+	skel = test_unpriv_map_access__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
+		return;
+
+	if (!ASSERT_OK_PTR(skel->kconfig, "skel_kconfig"))
+		goto cleanup;
+
+	if (!skel->kconfig->CONFIG_BPF_UNPRIV_MAP_ACCESS) {
+		printf("%s:SKIP:CONFIG_BPF_UNPRIV_MAP_ACCESS is not set", __func__);
+		test__skip();
+		goto cleanup;
+	}
+
+	skel->bss->perfbuf_val = 1;
+	skel->bss->ringbuf_val = 2;
+	skel->bss->test_pid = getpid();
+
+	if (!ASSERT_OK(test_unpriv_map_access__attach(skel), "skel_attach"))
+		goto cleanup;
+
+	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 id disabled */
+	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;
+	}
+
+	if (!ASSERT_OK(cap_disable_effective(ADMIN_CAPS, &save_caps), "disable caps"))
+		goto cleanup;
+
+	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, "bpf_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];
+		int j;
+
+		/* skip ringbuf, perfbuf */
+		if (buf)
+			continue;
+
+		for (j = 0; j < nr_cpus; j++)
+			vals[j] = 1;
+
+		if (prog_array) {
+			ASSERT_EQ(bpf_map_update_elem(map_fds[i], &key, vals, 0), -EPERM,
+				  "bpf_map_update_elem_fail");
+			ASSERT_EQ(bpf_map_lookup_elem(map_fds[i], &key, &lookup_vals), -EPERM,
+				  "bpf_map_lookup_elem_fail");
+		} else {
+			ASSERT_OK(bpf_map_update_elem(map_fds[i], &key, vals, 0),
+				  "bpf_map_update_elem");
+			ASSERT_OK(bpf_map_lookup_elem(map_fds[i], &key, &lookup_vals),
+				  "bpf_map_lookup_elem");
+			ASSERT_EQ(lookup_vals[0], 1, "bpf_map_lookup_elem_values");
+			if (!array)
+				ASSERT_OK(bpf_map_delete_elem(map_fds[i], &key),
+					  "bpf_map_delete_elem");
+		}
+	}
+cleanup:
+	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);
+	if (perfbuf)
+		perf_buffer__free(perfbuf);
+	if (ringbuf)
+		ring_buffer__free(ringbuf);
+	for (i = 0; i < ARRAY_SIZE(map_paths); i++)
+		unlink(map_paths[i]);
+	test_unpriv_map_access__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_unpriv_map_access.c b/tools/testing/selftests/bpf/progs/test_unpriv_map_access.c
new file mode 100644
index 000000000000..a215e2f1fa16
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_unpriv_map_access.c
@@ -0,0 +1,81 @@
+// 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"
+
+extern bool CONFIG_BPF_UNPRIV_MAP_ACCESS __kconfig;
+bool unpriv_map_access = 0;
+__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_enter(void *ctx)
+{
+	int cur_pid;
+
+	cur_pid = bpf_get_current_pid_tgid() >> 32;
+
+	if (cur_pid != test_pid)
+		return 0;
+
+	unpriv_map_access = CONFIG_BPF_UNPRIV_MAP_ACCESS;
+
+	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;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.27.0

Re: [RFC bpf-next 0/2] bpf: allow unprivileged map access to some map types

[Alexei Starovoitov]

On Wed, May 11, 2022 at 02:19:26PM +0100, Alan Maguire wrote:
> 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 use cases
> prior to having unpriviliged_bpf_disabled set to >= 1 however,
> it made 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, after it has been loaded, interacts with it via
> pinned maps.
> 
> Such a split model is not possible with unprivileged BPF disabled,
> since even those activities such as map interactions, retrieving
> map information from pinned paths etc are blocked to
> unprivileged users.  However, granting CAP_BPF to such unprivileged
> users is quite a big hammer, allowing them to do pretty much
> anything with BPF.
> 
> This very rough RFC explores the idea - with
> CONFIG_BPF_UNPRIV_MAP_ACCESS=y - of allowing unprivileged processes
> to retrieve and work with a restricted set of BPF maps.  See
> patch 1 for the restrictions on BPF cmd and map type. Note that
> permission checks on maps are still enforced, so it's still
> possible to prevent unwanted interference by unprivileged users
> by pinning a map and setting permissions to prevent access.
> CONFIG_BPF_UNPRIV_MAP_ACCESS defaults to n, preserving current
> behaviour of blocking all BPF syscall commands.
> 
> Discussion on the bpf mailing list already alluded to this idea [1],
> though it's possible I misinterpreted it.

Thanks for the follow up. We had a long discussion about this during lsfmmbpf.
In short a bunch of folks wants to address this problem.
Your summary of the problem is accurate.
The patch though is overly cautious in fixing the issue.
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.
Additional kconfig is unnecessary. Also no need to filter
different map types. Only array/hash/ringbuf are ok for unpriv
when that sysctl is off. The rest of maps have their own cap_bpf checks
at creation time which is enough.
The patch 1 should probably be something like:
  if (!capable &&
      (cmd == BPF_MAP_CREATE || cmd == BPF_PROG_LOAD))
    return -EPERM;
For all other commands the user has to have a valid map/btf/prog FD to proceed.
There are few special commands that don't need to be in the above 'if':
. BPF_[PROG|MAP|BTF]_GET_NEXT_ID have explicit cap_sys_admin check.
. BPF_OBJ_GET is using traditional file ACLs to access.
. BPF_BTF_LOAD has its own cap_bpf check.

Of course there could be bugs in any of unpriv code paths, but they were
enabled with sysctl off for long time. When/if new bugs are found they will be fixed.
The unprivileged_bpf_disabled's default was flipped only because of HW bugs.
In all HW spectre exploits BPF_PROG_LOAD was the mandatory command.
Just disabling that command is enough. The BPF_MAP_CREATE alone
cannot be used in spectre-like attack. But map_create without prog_load
is a useless combination for unrpiv. So having sysctl affecting them
both makes sense from symmetry pov. libbpf and other loaders typically
create a map first, so with sysctl off the unpriv users will see those eperms first.
I hope it's mostly accurate summary of the discussion.