On Tue, Apr 11, 2023 at 09:32:55PM -0700, Andrii Nakryiko wrote: > This allows to do more centralized decisions later on, and generally > makes it very explicit which maps are privileged and which are not. > > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx> > [...] > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c > index 00c253b84bf5..c69db80fc947 100644 > --- a/kernel/bpf/hashtab.c > +++ b/kernel/bpf/hashtab.c > @@ -422,12 +422,6 @@ static int htab_map_alloc_check(union bpf_attr *attr) > BUILD_BUG_ON(offsetof(struct htab_elem, fnode.next) != > offsetof(struct htab_elem, hash_node.pprev)); > > - if (lru && !bpf_capable()) > - /* LRU implementation is much complicated than other > - * maps. Hence, limit to CAP_BPF. > - */ > - return -EPERM; > - The LRU part of this check gets lost, doesn't it? More specifically, doesn't this make the security check for htab_map_alloc_check() more strict than before? (If that's okay, please mention the logical change in the commit log.) > [...] > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index a090737f98ea..cbea4999e92f 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -1101,17 +1101,6 @@ static int map_create(union bpf_attr *attr) > int f_flags; > int err; > > - /* 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 (!bpf_capable() && sysctl_unprivileged_bpf_disabled) > - return -EPERM; > - Given that this was already performing a centralized capability check, why were the individual functions doing checks before too? (I'm wondering if the individual functions remain the better place to do this checking?) > err = CHECK_ATTR(BPF_MAP_CREATE); > if (err) > return -EINVAL; > @@ -1155,6 +1144,65 @@ static int map_create(union bpf_attr *attr) > ops = &bpf_map_offload_ops; > if (!ops->map_mem_usage) > return -EINVAL; > + > + /* 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 (!bpf_capable() && sysctl_unprivileged_bpf_disabled) > + return -EPERM; > + > + /* check privileged map type permissions */ > + switch (map_type) { > + case BPF_MAP_TYPE_SK_STORAGE: > + case BPF_MAP_TYPE_INODE_STORAGE: > + case BPF_MAP_TYPE_TASK_STORAGE: > + case BPF_MAP_TYPE_CGRP_STORAGE: > + case BPF_MAP_TYPE_BLOOM_FILTER: > + case BPF_MAP_TYPE_LPM_TRIE: > + case BPF_MAP_TYPE_REUSEPORT_SOCKARRAY: > + case BPF_MAP_TYPE_STACK_TRACE: > + case BPF_MAP_TYPE_QUEUE: > + case BPF_MAP_TYPE_STACK: > + case BPF_MAP_TYPE_LRU_HASH: > + case BPF_MAP_TYPE_LRU_PERCPU_HASH: > + case BPF_MAP_TYPE_STRUCT_OPS: > + case BPF_MAP_TYPE_CPUMAP: > + if (!bpf_capable()) > + return -EPERM; > + break; > + case BPF_MAP_TYPE_SOCKMAP: > + case BPF_MAP_TYPE_SOCKHASH: > + case BPF_MAP_TYPE_DEVMAP: > + case BPF_MAP_TYPE_DEVMAP_HASH: > + case BPF_MAP_TYPE_XSKMAP: > + if (!capable(CAP_NET_ADMIN)) > + return -EPERM; > + break; > + case BPF_MAP_TYPE_ARRAY: > + case BPF_MAP_TYPE_PERCPU_ARRAY: > + case BPF_MAP_TYPE_PROG_ARRAY: > + case BPF_MAP_TYPE_PERF_EVENT_ARRAY: > + case BPF_MAP_TYPE_CGROUP_ARRAY: > + case BPF_MAP_TYPE_ARRAY_OF_MAPS: > + case BPF_MAP_TYPE_HASH: > + case BPF_MAP_TYPE_PERCPU_HASH: > + case BPF_MAP_TYPE_HASH_OF_MAPS: > + case BPF_MAP_TYPE_RINGBUF: > + case BPF_MAP_TYPE_USER_RINGBUF: > + case BPF_MAP_TYPE_CGROUP_STORAGE: > + case BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE: > + /* unprivileged */ > + break; > + default: > + WARN(1, "unsupported map type %d", map_type); > + return -EPERM; Thank you for making sure this fails safe! :) > + } > + > map = ops->map_alloc(attr); > if (IS_ERR(map)) > return PTR_ERR(map); > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > index 7c189c2e2fbf..4b67bb5e7f9c 100644 > --- a/net/core/sock_map.c > +++ b/net/core/sock_map.c > @@ -32,8 +32,6 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr) > { > struct bpf_stab *stab; > > - if (!capable(CAP_NET_ADMIN)) > - return ERR_PTR(-EPERM); > if (attr->max_entries == 0 || > attr->key_size != 4 || > (attr->value_size != sizeof(u32) && > @@ -1085,8 +1083,6 @@ static struct bpf_map *sock_hash_alloc(union bpf_attr *attr) > struct bpf_shtab *htab; > int i, err; > > - if (!capable(CAP_NET_ADMIN)) > - return ERR_PTR(-EPERM); > if (attr->max_entries == 0 || > attr->key_size == 0 || > (attr->value_size != sizeof(u32) && > diff --git a/net/xdp/xskmap.c b/net/xdp/xskmap.c > index 2c1427074a3b..e1c526f97ce3 100644 > --- a/net/xdp/xskmap.c > +++ b/net/xdp/xskmap.c > @@ -5,7 +5,6 @@ > > #include <linux/bpf.h> > #include <linux/filter.h> > -#include <linux/capability.h> > #include <net/xdp_sock.h> > #include <linux/slab.h> > #include <linux/sched.h> > @@ -68,9 +67,6 @@ static struct bpf_map *xsk_map_alloc(union bpf_attr *attr) > int numa_node; > u64 size; > > - if (!capable(CAP_NET_ADMIN)) > - return ERR_PTR(-EPERM); > - > if (attr->max_entries == 0 || attr->key_size != 4 || > attr->value_size != 4 || > attr->map_flags & ~(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)) > diff --git a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c > index 8383a99f610f..0adf8d9475cb 100644 > --- a/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c > +++ b/tools/testing/selftests/bpf/prog_tests/unpriv_bpf_disabled.c > @@ -171,7 +171,11 @@ static void test_unpriv_bpf_disabled_negative(struct test_unpriv_bpf_disabled *s > prog_insns, prog_insn_cnt, &load_opts), > -EPERM, "prog_load_fails"); > > - for (i = BPF_MAP_TYPE_HASH; i <= BPF_MAP_TYPE_BLOOM_FILTER; i++) > + /* some map types require particular correct parameters which could be > + * sanity-checked before enforcing -EPERM, so only validate that > + * the simple ARRAY and HASH maps are failing with -EPERM > + */ > + for (i = BPF_MAP_TYPE_HASH; i <= BPF_MAP_TYPE_ARRAY; i++) > ASSERT_EQ(bpf_map_create(i, NULL, sizeof(int), sizeof(int), 1, NULL), > -EPERM, "map_create_fails"); > > -- > 2.34.1 > -- Kees Cook