| Message ID | 20230412043300.360803-8-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | New BPF map and BTF security LSM hooks | expand |
On Wed, Apr 12, 2023 at 12:33 AM Andrii Nakryiko <andrii@kernel.org> wrote: > > Add new LSM hook, bpf_btf_load_security, that allows custom LSM security > policies controlling BTF data loading permissions (BPF_BTF_LOAD command > of bpf() syscall) granularly and precisely. > > This complements bpf_map_create_security LSM hook added earlier and > follow the same semantics: 0 means perform standard kernel capabilities-based > checks, negative error rejects BTF object load, while positive one skips > CAP_BPF check and allows BTF data object creation. > > With this hook, together with bpf_map_create_security, we now can also allow > trusted unprivileged process to create BPF maps that require BTF, which > we take advantaged in the next patch to improve the coverage of added > BPF selftest. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > include/linux/lsm_hook_defs.h | 1 + > include/linux/lsm_hooks.h | 13 +++++++++++++ > include/linux/security.h | 6 ++++++ > kernel/bpf/bpf_lsm.c | 1 + > kernel/bpf/syscall.c | 10 ++++++++++ > security/security.c | 4 ++++ > 6 files changed, 35 insertions(+) ... > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 42d8473237ab..bbf70bddc770 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -4449,12 +4449,22 @@ static int bpf_obj_get_info_by_fd(const union bpf_attr *attr, > > static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size) > { > + int err; > + > if (CHECK_ATTR(BPF_BTF_LOAD)) > return -EINVAL; > > + /* security checks */ > + err = security_bpf_btf_load(attr); > + if (err < 0) > + return err; > + if (err > 0) > + goto skip_priv_checks; > + > if (!bpf_capable()) > return -EPERM; > > +skip_priv_checks: > return btf_new_fd(attr, uattr, uattr_size); > } Beyond the objection I brought up in the patchset cover letter, I believe the work of the security_bpf_btf_load() hook presented here could be done by the existing security_bpf() LSM hook. If you believe that not to be the case, please let me know.
On Wed, Apr 12, 2023 at 9:53 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Apr 12, 2023 at 12:33 AM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > Add new LSM hook, bpf_btf_load_security, that allows custom LSM security > > policies controlling BTF data loading permissions (BPF_BTF_LOAD command > > of bpf() syscall) granularly and precisely. > > > > This complements bpf_map_create_security LSM hook added earlier and > > follow the same semantics: 0 means perform standard kernel capabilities-based > > checks, negative error rejects BTF object load, while positive one skips > > CAP_BPF check and allows BTF data object creation. > > > > With this hook, together with bpf_map_create_security, we now can also allow > > trusted unprivileged process to create BPF maps that require BTF, which > > we take advantaged in the next patch to improve the coverage of added > > BPF selftest. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > include/linux/lsm_hook_defs.h | 1 + > > include/linux/lsm_hooks.h | 13 +++++++++++++ > > include/linux/security.h | 6 ++++++ > > kernel/bpf/bpf_lsm.c | 1 + > > kernel/bpf/syscall.c | 10 ++++++++++ > > security/security.c | 4 ++++ > > 6 files changed, 35 insertions(+) > > ... > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 42d8473237ab..bbf70bddc770 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -4449,12 +4449,22 @@ static int bpf_obj_get_info_by_fd(const union bpf_attr *attr, > > > > static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size) > > { > > + int err; > > + > > if (CHECK_ATTR(BPF_BTF_LOAD)) > > return -EINVAL; > > > > + /* security checks */ > > + err = security_bpf_btf_load(attr); > > + if (err < 0) > > + return err; > > + if (err > 0) > > + goto skip_priv_checks; > > + > > if (!bpf_capable()) > > return -EPERM; > > > > +skip_priv_checks: > > return btf_new_fd(attr, uattr, uattr_size); > > } > > Beyond the objection I brought up in the patchset cover letter, I > believe the work of the security_bpf_btf_load() hook presented here > could be done by the existing security_bpf() LSM hook. If you believe > that not to be the case, please let me know. security_bpf() could prevent BTF object loading only, but security_bpf_btf_load() can *also* allow *trusted* (according to LSM policy) unprivileged process to proceed. So it doesn't seem like they are interchangeable. > > -- > paul-moore.com
On Wed, Apr 12, 2023 at 9:43 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Wed, Apr 12, 2023 at 9:53 AM Paul Moore <paul@paul-moore.com> wrote: > > On Wed, Apr 12, 2023 at 12:33 AM Andrii Nakryiko <andrii@kernel.org> wrote: > > > > > > Add new LSM hook, bpf_btf_load_security, that allows custom LSM security > > > policies controlling BTF data loading permissions (BPF_BTF_LOAD command > > > of bpf() syscall) granularly and precisely. > > > > > > This complements bpf_map_create_security LSM hook added earlier and > > > follow the same semantics: 0 means perform standard kernel capabilities-based > > > checks, negative error rejects BTF object load, while positive one skips > > > CAP_BPF check and allows BTF data object creation. > > > > > > With this hook, together with bpf_map_create_security, we now can also allow > > > trusted unprivileged process to create BPF maps that require BTF, which > > > we take advantaged in the next patch to improve the coverage of added > > > BPF selftest. > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > include/linux/lsm_hook_defs.h | 1 + > > > include/linux/lsm_hooks.h | 13 +++++++++++++ > > > include/linux/security.h | 6 ++++++ > > > kernel/bpf/bpf_lsm.c | 1 + > > > kernel/bpf/syscall.c | 10 ++++++++++ > > > security/security.c | 4 ++++ > > > 6 files changed, 35 insertions(+) > > > > ... > > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > > index 42d8473237ab..bbf70bddc770 100644 > > > --- a/kernel/bpf/syscall.c > > > +++ b/kernel/bpf/syscall.c > > > @@ -4449,12 +4449,22 @@ static int bpf_obj_get_info_by_fd(const union bpf_attr *attr, > > > > > > static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size) > > > { > > > + int err; > > > + > > > if (CHECK_ATTR(BPF_BTF_LOAD)) > > > return -EINVAL; > > > > > > + /* security checks */ > > > + err = security_bpf_btf_load(attr); > > > + if (err < 0) > > > + return err; > > > + if (err > 0) > > > + goto skip_priv_checks; > > > + > > > if (!bpf_capable()) > > > return -EPERM; > > > > > > +skip_priv_checks: > > > return btf_new_fd(attr, uattr, uattr_size); > > > } > > > > Beyond the objection I brought up in the patchset cover letter, I > > believe the work of the security_bpf_btf_load() hook presented here > > could be done by the existing security_bpf() LSM hook. If you believe > > that not to be the case, please let me know. > > security_bpf() could prevent BTF object loading only, but > security_bpf_btf_load() can *also* allow *trusted* (according to LSM > policy) unprivileged process to proceed. So it doesn't seem like they > are interchangeable. As discussed in the cover letter thread, I'm opposed to using a LSM hook to skip/bypass/circumvent/etc. existing capability checks.
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index b4fe9ed7021a..92cb0f95b970 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -396,6 +396,7 @@ LSM_HOOK(void, LSM_RET_VOID, audit_rule_free, void *lsmrule) LSM_HOOK(int, 0, bpf, int cmd, union bpf_attr *attr, unsigned int size) LSM_HOOK(int, 0, bpf_map, struct bpf_map *map, fmode_t fmode) LSM_HOOK(int, 0, bpf_prog, struct bpf_prog *prog) +LSM_HOOK(int, 0, bpf_btf_load_security, const union bpf_attr *attr) LSM_HOOK(int, 0, bpf_map_create_security, const union bpf_attr *attr) LSM_HOOK(int, 0, bpf_map_alloc_security, struct bpf_map *map) LSM_HOOK(void, LSM_RET_VOID, bpf_map_free_security, struct bpf_map *map) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 42bf7c0aa4d8..cde96b5e15e2 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1598,6 +1598,19 @@ * @prog: bpf prog that userspace want to use. * Return 0 if permission is granted. * + * @bpf_btf_load_security: + * Do a check to determine permission to create BTF data object + * (BPF_BTF_LOAD command of bpf() syscall). + * Implementation can override kernel capabilities checks according to + * the rules below: + * - 0 should be returned to delegate permission checks to other + * installed LSM callbacks and/or hard-wired kernel logic, which + * would enforce CAP_BPF capability; + * - reject BTF data object creation by returning -EPERM or any other + * negative error code; + * - allow BTF data object creation, overriding kernel checks, by + * returning a positive result. + * * @bpf_map_create_security: * Do a check to determine permission to create requested BPF map. * Implementation can override kernel capabilities checks according to diff --git a/include/linux/security.h b/include/linux/security.h index e5374fe92ef6..f3ee1800392d 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -2023,6 +2023,7 @@ struct bpf_prog_aux; extern int security_bpf(int cmd, union bpf_attr *attr, unsigned int size); extern int security_bpf_map(struct bpf_map *map, fmode_t fmode); extern int security_bpf_prog(struct bpf_prog *prog); +extern int security_bpf_btf_load(const union bpf_attr *attr); extern int security_bpf_map_create(const union bpf_attr *attr); extern int security_bpf_map_alloc(struct bpf_map *map); extern void security_bpf_map_free(struct bpf_map *map); @@ -2045,6 +2046,11 @@ static inline int security_bpf_prog(struct bpf_prog *prog) return 0; } +static inline int security_bpf_btf_load(const union bpf_attr *attr) +{ + return 0; +} + static inline int security_bpf_map_create(const union bpf_attr *attr) { return 0; diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c index 931d4dda5dac..53c39a18fd2c 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -260,6 +260,7 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) BTF_SET_START(sleepable_lsm_hooks) BTF_ID(func, bpf_lsm_bpf) BTF_ID(func, bpf_lsm_bpf_map) +BTF_ID(func, bpf_lsm_bpf_btf_load_security) BTF_ID(func, bpf_lsm_bpf_map_create_security) BTF_ID(func, bpf_lsm_bpf_map_alloc_security) BTF_ID(func, bpf_lsm_bpf_map_free_security) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 42d8473237ab..bbf70bddc770 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -4449,12 +4449,22 @@ static int bpf_obj_get_info_by_fd(const union bpf_attr *attr, static int bpf_btf_load(const union bpf_attr *attr, bpfptr_t uattr, __u32 uattr_size) { + int err; + if (CHECK_ATTR(BPF_BTF_LOAD)) return -EINVAL; + /* security checks */ + err = security_bpf_btf_load(attr); + if (err < 0) + return err; + if (err > 0) + goto skip_priv_checks; + if (!bpf_capable()) return -EPERM; +skip_priv_checks: return btf_new_fd(attr, uattr, uattr_size); } diff --git a/security/security.c b/security/security.c index f9b885680966..8869802ef5f5 100644 --- a/security/security.c +++ b/security/security.c @@ -2682,6 +2682,10 @@ int security_bpf_prog(struct bpf_prog *prog) { return call_int_hook(bpf_prog, 0, prog); } +int security_bpf_btf_load(const union bpf_attr *attr) +{ + return call_int_hook(bpf_btf_load_security, 0, attr); +} int security_bpf_map_create(const union bpf_attr *attr) { return call_int_hook(bpf_map_create_security, 0, attr);
Add new LSM hook, bpf_btf_load_security, that allows custom LSM security policies controlling BTF data loading permissions (BPF_BTF_LOAD command of bpf() syscall) granularly and precisely. This complements bpf_map_create_security LSM hook added earlier and follow the same semantics: 0 means perform standard kernel capabilities-based checks, negative error rejects BTF object load, while positive one skips CAP_BPF check and allows BTF data object creation. With this hook, together with bpf_map_create_security, we now can also allow trusted unprivileged process to create BPF maps that require BTF, which we take advantaged in the next patch to improve the coverage of added BPF selftest. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- include/linux/lsm_hook_defs.h | 1 + include/linux/lsm_hooks.h | 13 +++++++++++++ include/linux/security.h | 6 ++++++ kernel/bpf/bpf_lsm.c | 1 + kernel/bpf/syscall.c | 10 ++++++++++ security/security.c | 4 ++++ 6 files changed, 35 insertions(+)