Re: [PATCH v9 11/17] bpf,lsm: add BPF token LSM hooks
From: Paul Moore <paul@paul-moore.com>
Date: 2023-11-06 05:01:28
Also in:
bpf, linux-fsdevel, linux-security-module
On Nov 3, 2023 Andrii Nakryiko [off-list ref] wrote:
Wire up bpf_token_create and bpf_token_free LSM hooks, which allow to allocate LSM security blob (we add `void *security` field to struct bpf_token for that), but also control who can instantiate BPF token. This follows existing pattern for BPF map and BPF prog. Also add security_bpf_token_allow_cmd() and security_bpf_token_capable() LSM hooks that allow LSM implementation to control and negate (if necessary) BPF token's delegation of a specific bpf_cmd and capability, respectively. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- include/linux/bpf.h | 3 ++ include/linux/lsm_hook_defs.h | 5 +++ include/linux/security.h | 25 +++++++++++++++ kernel/bpf/bpf_lsm.c | 4 +++ kernel/bpf/token.c | 13 ++++++-- security/security.c | 60 +++++++++++++++++++++++++++++++++++ 6 files changed, 107 insertions(+), 3 deletions(-)
...
quoted hunk ↗ jump to hunk
diff --git a/include/linux/security.h b/include/linux/security.h index 08fd777cbe94..1d6edbf45d1c 100644 --- a/include/linux/security.h +++ b/include/linux/security.h@@ -60,6 +60,7 @@ struct fs_parameter; enum fs_value_type; struct watch; struct watch_notification; +enum bpf_cmd;
Yes, I think it's fine to include bpf.h in security.h instead of the forward declaration.
quoted hunk ↗ jump to hunk
/* Default (no) options for the capable function */ #define CAP_OPT_NONE 0x0@@ -2031,6 +2032,11 @@ extern void security_bpf_map_free(struct bpf_map *map); extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, struct bpf_token *token); extern void security_bpf_prog_free(struct bpf_prog *prog); +extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, + struct path *path); +extern void security_bpf_token_free(struct bpf_token *token); +extern int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd); +extern int security_bpf_token_capable(const struct bpf_token *token, int cap); #else static inline int security_bpf(int cmd, union bpf_attr *attr, unsigned int size)@@ -2065,6 +2071,25 @@ static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr * static inline void security_bpf_prog_free(struct bpf_prog *prog) { } + +static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, + struct path *path) +{ + return 0; +} + +static inline void security_bpf_token_free(struct bpf_token *token) +{ } + +static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) +{ + return 0; +} + +static inline int security_bpf_token_capable(const struct bpf_token *token, int cap) +{ + return 0; +}
Another nitpick, but I would prefer to shorten security_bpf_token_allow_cmd() renamed to security_bpf_token_cmd() both to shorten the name and to better fit convention. I realize the caller is named bpf_token_allow_cmd() but I'd still rather see the LSM hook with the shorter name.
quoted hunk ↗ jump to hunk
diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c index 35e6f55c2a41..5d04da54faea 100644 --- a/kernel/bpf/token.c +++ b/kernel/bpf/token.c@@ -7,11 +7,12 @@ #include <linux/idr.h> #include <linux/namei.h> #include <linux/user_namespace.h> +#include <linux/security.h> bool bpf_token_capable(const struct bpf_token *token, int cap) { /* BPF token allows ns_capable() level of capabilities */ - if (token) { + if (token && security_bpf_token_capable(token, cap) == 0) { if (ns_capable(token->userns, cap)) return true; if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN))
We typically perform the capability based access controls prior to the
LSM controls, meaning if we want to the token controls to work in a
similar way we should do something like this:
bool bpf_token_capable(...)
{
if (token) {
if (ns_capable(token, cap) ||
(cap != ADMIN && ns_capable(token, ADMIN)))
return security_bpf_token_capable(token, cap);
}
return capable(cap) || (cap != ADMIN && capable(...))
}
quoted hunk ↗ jump to hunk
@@ -28,6 +29,7 @@ void bpf_token_inc(struct bpf_token *token) static void bpf_token_free(struct bpf_token *token) { + security_bpf_token_free(token); put_user_ns(token->userns); kvfree(token); }@@ -172,6 +174,10 @@ int bpf_token_create(union bpf_attr *attr) token->allowed_progs = mnt_opts->delegate_progs; token->allowed_attachs = mnt_opts->delegate_attachs; + err = security_bpf_token_create(token, attr, &path); + if (err) + goto out_token; + fd = get_unused_fd_flags(O_CLOEXEC); if (fd < 0) { err = fd;@@ -216,8 +222,9 @@ bool bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) { if (!token) return false; - - return token->allowed_cmds & (1ULL << cmd); + if (!(token->allowed_cmds & (1ULL << cmd))) + return false; + return security_bpf_token_allow_cmd(token, cmd) == 0;
I'm not sure how much it really matters, but someone might prefer the '!!' approach/style over '== 0'.
} bool bpf_token_allow_map_type(const struct bpf_token *token, enum bpf_map_type type)
-- paul-moore.com