From: Kees Cook <keescook@chromium.org>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, daniel@iogearbox.net,
kpsingh@kernel.org, paul@paul-moore.com,
linux-security-module@vger.kernel.org
Subject: Re: [PATCH bpf-next 3/8] bpf: centralize permissions checks for all BPF map types
Date: Wed, 12 Apr 2023 11:01:16 -0700 [thread overview]
Message-ID: <6436f1ed.170a0220.6cc4d.79f3@mx.google.com> (raw)
In-Reply-To: <20230412043300.360803-4-andrii@kernel.org>
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@kernel.org>
> [...]
> 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
next prev parent reply other threads:[~2023-04-12 18:01 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-12 4:32 [PATCH bpf-next 0/8] New BPF map and BTF security LSM hooks Andrii Nakryiko
2023-04-12 4:32 ` [PATCH bpf-next 1/8] bpf: move unprivileged checks into map_create() and bpf_prog_load() Andrii Nakryiko
2023-04-12 17:49 ` Kees Cook
2023-04-13 0:22 ` Andrii Nakryiko
2023-04-12 4:32 ` [PATCH bpf-next 2/8] bpf: inline map creation logic in map_create() function Andrii Nakryiko
2023-04-12 17:53 ` Kees Cook
2023-04-13 0:22 ` Andrii Nakryiko
2023-04-12 4:32 ` [PATCH bpf-next 3/8] bpf: centralize permissions checks for all BPF map types Andrii Nakryiko
2023-04-12 18:01 ` Kees Cook [this message]
2023-04-13 0:23 ` Andrii Nakryiko
2023-04-12 4:32 ` [PATCH bpf-next 4/8] bpf, lsm: implement bpf_map_create_security LSM hook Andrii Nakryiko
2023-04-12 18:20 ` Kees Cook
2023-04-13 0:23 ` Andrii Nakryiko
2023-04-12 4:32 ` [PATCH bpf-next 5/8] selftests/bpf: validate new " Andrii Nakryiko
2023-04-12 18:23 ` Kees Cook
2023-04-13 0:23 ` Andrii Nakryiko
2023-04-12 4:32 ` [PATCH bpf-next 6/8] bpf: drop unnecessary bpf_capable() check in BPF_MAP_FREEZE command Andrii Nakryiko
2023-04-12 18:24 ` Kees Cook
2023-04-13 0:17 ` Andrii Nakryiko
2023-04-12 4:32 ` [PATCH bpf-next 7/8] bpf, lsm: implement bpf_btf_load_security LSM hook Andrii Nakryiko
2023-04-12 16:52 ` Paul Moore
2023-04-13 1:43 ` Andrii Nakryiko
2023-04-13 2:47 ` Paul Moore
2023-04-12 4:33 ` [PATCH bpf-next 8/8] selftests/bpf: enhance lsm_map_create test with BTF LSM control Andrii Nakryiko
2023-04-12 16:49 ` [PATCH bpf-next 0/8] New BPF map and BTF security LSM hooks Paul Moore
2023-04-12 17:47 ` Kees Cook
2023-04-12 18:06 ` Paul Moore
2023-04-12 18:28 ` Kees Cook
2023-04-12 19:06 ` Paul Moore
2023-04-13 1:43 ` Andrii Nakryiko
2023-04-13 2:56 ` Paul Moore
2023-04-13 5:16 ` Andrii Nakryiko
2023-04-13 15:11 ` Paul Moore
2023-04-17 23:29 ` Andrii Nakryiko
2023-04-18 0:47 ` Casey Schaufler
2023-04-21 0:00 ` Andrii Nakryiko
2023-04-18 14:21 ` Paul Moore
2023-04-21 0:00 ` Andrii Nakryiko
2023-04-21 18:57 ` Kees Cook
2023-04-13 16:54 ` Casey Schaufler
2023-04-17 23:31 ` Andrii Nakryiko
2023-04-13 19:03 ` Jonathan Corbet
2023-04-17 23:28 ` Andrii Nakryiko
2023-04-13 16:27 ` Casey Schaufler
2023-04-17 23:31 ` Andrii Nakryiko
2023-04-17 23:53 ` Casey Schaufler
2023-04-18 0:28 ` Andrii Nakryiko
2023-04-18 0:52 ` Casey Schaufler
2023-04-12 18:38 ` Casey Schaufler
2023-04-14 20:23 ` Dr. Greg
2023-04-17 23:31 ` Andrii Nakryiko
2023-04-19 10:53 ` Dr. Greg
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6436f1ed.170a0220.6cc4d.79f3@mx.google.com \
--to=keescook@chromium.org \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kpsingh@kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=paul@paul-moore.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).