| Message ID | 20200326142823.26277-5-kpsingh@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | MAC and Audit policy using eBPF (KRSI) | expand |
On Thu, Mar 26, 2020 at 7:29 AM KP Singh <kpsingh@chromium.org> wrote: > > From: KP Singh <kpsingh@google.com> > > JITed BPF programs are dynamically attached to the LSM hooks > using BPF trampolines. The trampoline prologue generates code to handle > conversion of the signature of the hook to the appropriate BPF context. > > The allocated trampoline programs are attached to the nop functions > initialized as LSM hooks. > > BPF_PROG_TYPE_LSM programs must have a GPL compatible license and > and need CAP_SYS_ADMIN (required for loading eBPF programs). > > Upon attachment: > > * A BPF fexit trampoline is used for LSM hooks with a void return type. > * A BPF fmod_ret trampoline is used for LSM hooks which return an > int. The attached programs can override the return value of the > bpf LSM hook to indicate a MAC Policy decision. > > Signed-off-by: KP Singh <kpsingh@google.com> > Reviewed-by: Brendan Jackman <jackmanb@google.com> > Reviewed-by: Florent Revest <revest@google.com> > --- Acked-by: Andrii Nakryiko <andriin@fb.com> > include/linux/bpf_lsm.h | 11 ++++++++ > kernel/bpf/bpf_lsm.c | 28 ++++++++++++++++++++ > kernel/bpf/btf.c | 9 ++++++- > kernel/bpf/syscall.c | 57 ++++++++++++++++++++++++++++------------- > kernel/bpf/trampoline.c | 17 +++++++++--- > kernel/bpf/verifier.c | 19 +++++++++++--- > 6 files changed, 114 insertions(+), 27 deletions(-) > [...] > @@ -2479,6 +2496,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) > } > buf[sizeof(buf) - 1] = 0; > tp_name = buf; > + break; > + default: > + err = -EINVAL; > + goto out_put_prog; > } is indentation off here or it's my email client? [...]
On 26-Mär 12:12, Andrii Nakryiko wrote: > On Thu, Mar 26, 2020 at 7:29 AM KP Singh <kpsingh@chromium.org> wrote: > > > > From: KP Singh <kpsingh@google.com> > > > > JITed BPF programs are dynamically attached to the LSM hooks > > using BPF trampolines. The trampoline prologue generates code to handle > > conversion of the signature of the hook to the appropriate BPF context. > > > > The allocated trampoline programs are attached to the nop functions > > initialized as LSM hooks. > > > > BPF_PROG_TYPE_LSM programs must have a GPL compatible license and > > and need CAP_SYS_ADMIN (required for loading eBPF programs). > > > > Upon attachment: > > > > * A BPF fexit trampoline is used for LSM hooks with a void return type. > > * A BPF fmod_ret trampoline is used for LSM hooks which return an > > int. The attached programs can override the return value of the > > bpf LSM hook to indicate a MAC Policy decision. > > > > Signed-off-by: KP Singh <kpsingh@google.com> > > Reviewed-by: Brendan Jackman <jackmanb@google.com> > > Reviewed-by: Florent Revest <revest@google.com> > > --- > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > > include/linux/bpf_lsm.h | 11 ++++++++ > > kernel/bpf/bpf_lsm.c | 28 ++++++++++++++++++++ > > kernel/bpf/btf.c | 9 ++++++- > > kernel/bpf/syscall.c | 57 ++++++++++++++++++++++++++++------------- > > kernel/bpf/trampoline.c | 17 +++++++++--- > > kernel/bpf/verifier.c | 19 +++++++++++--- > > 6 files changed, 114 insertions(+), 27 deletions(-) > > > > [...] > > > @@ -2479,6 +2496,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) > > } > > buf[sizeof(buf) - 1] = 0; > > tp_name = buf; > > + break; > > + default: > > + err = -EINVAL; > > + goto out_put_prog; > > } > > is indentation off here or it's my email client? You're mail client is fine :) It's me. - KP > > [...]
On Thu, 26 Mar 2020, KP Singh wrote: > +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, > + const struct bpf_prog *prog) > +{ > + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks > + */ > + if (!capable(CAP_MAC_ADMIN)) > + return -EPERM; > + Stephen, can you confirm that your concerns around this are resolved (IIRC, by SELinux implementing a bpf_prog callback) ?
On Thu, Mar 26, 2020 at 03:28:19PM +0100, KP Singh wrote: > > if (arg == nr_args) { > - if (prog->expected_attach_type == BPF_TRACE_FEXIT) { > + /* BPF_LSM_MAC programs only have int and void functions they > + * can be attached to. When they are attached to a void function > + * they result in the creation of an FEXIT trampoline and when > + * to a function that returns an int, a MODIFY_RETURN > + * trampoline. > + */ > + if (prog->expected_attach_type == BPF_TRACE_FEXIT || > + prog->expected_attach_type == BPF_LSM_MAC) { > if (!t) > return true; > t = btf_type_by_id(btf, t->type); Could you add a comment here that though BPF_MODIFY_RETURN-like check if (ret_type != 'int') return -EINVAL; is _not_ done here. It is still safe, since LSM hooks have only void and int return types. > + case BPF_LSM_MAC: > + if (!prog->aux->attach_func_proto->type) > + /* The function returns void, we cannot modify its > + * return value. > + */ > + return BPF_TRAMP_FEXIT; > + else > + return BPF_TRAMP_MODIFY_RETURN; I was thinking whether it would help performance significantly enough if we add a flavor of BPF_TRAMP_FEXIT that doesn't have BPF_TRAMP_F_CALL_ORIG. That will save the cost of nop call, but I guess indirect call due to lsm infra is slow enough, so this extra few cycles won't be noticeable. So I'm fine with it as-is. When lsm hooks will get rid of indirect call we can optimize it further.
On 3/26/20 8:24 PM, James Morris wrote: > On Thu, 26 Mar 2020, KP Singh wrote: > >> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, >> + const struct bpf_prog *prog) >> +{ >> + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks >> + */ >> + if (!capable(CAP_MAC_ADMIN)) >> + return -EPERM; >> + > > Stephen, can you confirm that your concerns around this are resolved > (IIRC, by SELinux implementing a bpf_prog callback) ? I guess the only residual concern I have is that CAP_MAC_ADMIN means something different to SELinux (ability to get/set file security contexts unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check here (versus calling a new security hook here and checking CAP_MAC_ADMIN in the implementation of that hook for the modules that want that) conflates two very different things. Prior to this patch, there are no users of CAP_MAC_ADMIN outside of individual security modules; it is only checked in module-specific logic within apparmor, safesetid, selinux, and smack, so the meaning was module-specific.
On 27-Mär 08:27, Stephen Smalley wrote: > On 3/26/20 8:24 PM, James Morris wrote: > > On Thu, 26 Mar 2020, KP Singh wrote: > > > > > +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, > > > + const struct bpf_prog *prog) > > > +{ > > > + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks > > > + */ > > > + if (!capable(CAP_MAC_ADMIN)) > > > + return -EPERM; > > > + > > > > Stephen, can you confirm that your concerns around this are resolved > > (IIRC, by SELinux implementing a bpf_prog callback) ? > > I guess the only residual concern I have is that CAP_MAC_ADMIN means > something different to SELinux (ability to get/set file security contexts > unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check > here (versus calling a new security hook here and checking CAP_MAC_ADMIN in > the implementation of that hook for the modules that want that) conflates > two very different things. Prior to this patch, there are no users of > CAP_MAC_ADMIN outside of individual security modules; it is only checked in > module-specific logic within apparmor, safesetid, selinux, and smack, so the > meaning was module-specific. As we had discussed, We do have a security hook as well: https://lore.kernel.org/bpf/20200324180652.GA11855@chromium.org/ The bpf_prog hook which can check for BPF_PROG_TYPE_LSM and implement module specific logic for LSM programs. I thougt that was okay? Kees was in favor of keeping the CAP_MAC_ADMIN check here: https://lore.kernel.org/bpf/202003241133.16C02BE5B@keescook If you feel strongly and Kees agrees, we can remove the CAP_MAC_ADMIN check here, but given that we already have a security hook that meets the requirements, we probably don't need another one. - KP > > > >
On 3/27/20 8:41 AM, KP Singh wrote: > On 27-Mär 08:27, Stephen Smalley wrote: >> On 3/26/20 8:24 PM, James Morris wrote: >>> On Thu, 26 Mar 2020, KP Singh wrote: >>> >>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, >>>> + const struct bpf_prog *prog) >>>> +{ >>>> + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks >>>> + */ >>>> + if (!capable(CAP_MAC_ADMIN)) >>>> + return -EPERM; >>>> + >>> >>> Stephen, can you confirm that your concerns around this are resolved >>> (IIRC, by SELinux implementing a bpf_prog callback) ? >> >> I guess the only residual concern I have is that CAP_MAC_ADMIN means >> something different to SELinux (ability to get/set file security contexts >> unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check >> here (versus calling a new security hook here and checking CAP_MAC_ADMIN in >> the implementation of that hook for the modules that want that) conflates >> two very different things. Prior to this patch, there are no users of >> CAP_MAC_ADMIN outside of individual security modules; it is only checked in >> module-specific logic within apparmor, safesetid, selinux, and smack, so the >> meaning was module-specific. > > As we had discussed, We do have a security hook as well: > > https://lore.kernel.org/bpf/20200324180652.GA11855@chromium.org/ > > The bpf_prog hook which can check for BPF_PROG_TYPE_LSM and implement > module specific logic for LSM programs. I thougt that was okay? > > Kees was in favor of keeping the CAP_MAC_ADMIN check here: > > https://lore.kernel.org/bpf/202003241133.16C02BE5B@keescook > > If you feel strongly and Kees agrees, we can remove the CAP_MAC_ADMIN > check here, but given that we already have a security hook that meets > the requirements, we probably don't need another one. I would favor removing the CAP_MAC_ADMIN check here, and implementing it in a bpf_prog hook for Smack and AppArmor if they want that. SELinux would implement its own check in its existing bpf_prog hook.
On 27-Mär 09:43, Stephen Smalley wrote: > On 3/27/20 8:41 AM, KP Singh wrote: > > On 27-Mär 08:27, Stephen Smalley wrote: > > > On 3/26/20 8:24 PM, James Morris wrote: > > > > On Thu, 26 Mar 2020, KP Singh wrote: > > > > > > > > > +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, > > > > > + const struct bpf_prog *prog) > > > > > +{ > > > > > + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks > > > > > + */ > > > > > + if (!capable(CAP_MAC_ADMIN)) > > > > > + return -EPERM; > > > > > + > > > > > > > > Stephen, can you confirm that your concerns around this are resolved > > > > (IIRC, by SELinux implementing a bpf_prog callback) ? > > > > > > I guess the only residual concern I have is that CAP_MAC_ADMIN means > > > something different to SELinux (ability to get/set file security contexts > > > unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check > > > here (versus calling a new security hook here and checking CAP_MAC_ADMIN in > > > the implementation of that hook for the modules that want that) conflates > > > two very different things. Prior to this patch, there are no users of > > > CAP_MAC_ADMIN outside of individual security modules; it is only checked in > > > module-specific logic within apparmor, safesetid, selinux, and smack, so the > > > meaning was module-specific. > > > > As we had discussed, We do have a security hook as well: > > > > https://lore.kernel.org/bpf/20200324180652.GA11855@chromium.org/ > > > > The bpf_prog hook which can check for BPF_PROG_TYPE_LSM and implement > > module specific logic for LSM programs. I thougt that was okay? > > > > Kees was in favor of keeping the CAP_MAC_ADMIN check here: > > > > https://lore.kernel.org/bpf/202003241133.16C02BE5B@keescook > > > > If you feel strongly and Kees agrees, we can remove the CAP_MAC_ADMIN > > check here, but given that we already have a security hook that meets > > the requirements, we probably don't need another one. > > I would favor removing the CAP_MAC_ADMIN check here, and implementing it in Okay. For the scope of this series I will remove this check in the next revision. If people feel strongly that we need it centrally within the BPF infrastructure, we can do that as a separate patch and discuss it there. > a bpf_prog hook for Smack and AppArmor if they want that. SELinux would > implement its own check in its existing bpf_prog hook. I think Smack and AppArmor can also use the same hook. Since we already have a hook, I don't think anyone is blocked from implementing policy logic for loading LSM BPF programs. James/Kees does this sound okay? - KP > > >
On 26-Mär 20:12, Alexei Starovoitov wrote: > On Thu, Mar 26, 2020 at 03:28:19PM +0100, KP Singh wrote: > > > > if (arg == nr_args) { > > - if (prog->expected_attach_type == BPF_TRACE_FEXIT) { > > + /* BPF_LSM_MAC programs only have int and void functions they > > + * can be attached to. When they are attached to a void function > > + * they result in the creation of an FEXIT trampoline and when > > + * to a function that returns an int, a MODIFY_RETURN > > + * trampoline. > > + */ > > + if (prog->expected_attach_type == BPF_TRACE_FEXIT || > > + prog->expected_attach_type == BPF_LSM_MAC) { > > if (!t) > > return true; > > t = btf_type_by_id(btf, t->type); > > Could you add a comment here that though BPF_MODIFY_RETURN-like check > if (ret_type != 'int') return -EINVAL; > is _not_ done here. It is still safe, since LSM hooks have only > void and int return types. Good idea, I reworded the comment to make this explicit and moved the comment to inside the if condition. > > > + case BPF_LSM_MAC: > > + if (!prog->aux->attach_func_proto->type) > > + /* The function returns void, we cannot modify its > > + * return value. > > + */ > > + return BPF_TRAMP_FEXIT; > > + else > > + return BPF_TRAMP_MODIFY_RETURN; > > I was thinking whether it would help performance significantly enough > if we add a flavor of BPF_TRAMP_FEXIT that doesn't have > BPF_TRAMP_F_CALL_ORIG. Agreed. > That will save the cost of nop call, but I guess indirect call due > to lsm infra is slow enough, so this extra few cycles won't be noticeable. > So I'm fine with it as-is. When lsm hooks will get rid of indirect call > we can optimize it further. Also agreed, that's the next step. :) - KP
On 3/27/2020 6:43 AM, Stephen Smalley wrote: > On 3/27/20 8:41 AM, KP Singh wrote: >> On 27-Mär 08:27, Stephen Smalley wrote: >>> On 3/26/20 8:24 PM, James Morris wrote: >>>> On Thu, 26 Mar 2020, KP Singh wrote: >>>> >>>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, >>>>> + const struct bpf_prog *prog) >>>>> +{ >>>>> + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks >>>>> + */ >>>>> + if (!capable(CAP_MAC_ADMIN)) >>>>> + return -EPERM; >>>>> + >>>> >>>> Stephen, can you confirm that your concerns around this are resolved >>>> (IIRC, by SELinux implementing a bpf_prog callback) ? >>> >>> I guess the only residual concern I have is that CAP_MAC_ADMIN means >>> something different to SELinux (ability to get/set file security contexts >>> unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check >>> here (versus calling a new security hook here and checking CAP_MAC_ADMIN in >>> the implementation of that hook for the modules that want that) conflates >>> two very different things. Prior to this patch, there are no users of >>> CAP_MAC_ADMIN outside of individual security modules; it is only checked in >>> module-specific logic within apparmor, safesetid, selinux, and smack, so the >>> meaning was module-specific. >> >> As we had discussed, We do have a security hook as well: >> >> https://lore.kernel.org/bpf/20200324180652.GA11855@chromium.org/ >> >> The bpf_prog hook which can check for BPF_PROG_TYPE_LSM and implement >> module specific logic for LSM programs. I thougt that was okay? >> >> Kees was in favor of keeping the CAP_MAC_ADMIN check here: >> >> https://lore.kernel.org/bpf/202003241133.16C02BE5B@keescook >> >> If you feel strongly and Kees agrees, we can remove the CAP_MAC_ADMIN >> check here, but given that we already have a security hook that meets >> the requirements, we probably don't need another one. > > I would favor removing the CAP_MAC_ADMIN check here, and implementing it in a bpf_prog hook for Smack and AppArmor if they want that. SELinux would implement its own check in its existing bpf_prog hook. > The whole notion of one security module calling into another for permission to do something still gives me the heebee jeebees, but if more nimble minds than mine think this is a good idea I won't nack it.
On Fri, Mar 27, 2020 at 09:36:15AM -0700, Casey Schaufler wrote: > On 3/27/2020 6:43 AM, Stephen Smalley wrote: > > On 3/27/20 8:41 AM, KP Singh wrote: > >> On 27-Mär 08:27, Stephen Smalley wrote: > >>> On 3/26/20 8:24 PM, James Morris wrote: > >>>> On Thu, 26 Mar 2020, KP Singh wrote: > >>>> > >>>>> +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, > >>>>> + const struct bpf_prog *prog) > >>>>> +{ > >>>>> + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks > >>>>> + */ > >>>>> + if (!capable(CAP_MAC_ADMIN)) > >>>>> + return -EPERM; > >>>>> + > >>>> > >>>> Stephen, can you confirm that your concerns around this are resolved > >>>> (IIRC, by SELinux implementing a bpf_prog callback) ? > >>> > >>> I guess the only residual concern I have is that CAP_MAC_ADMIN means > >>> something different to SELinux (ability to get/set file security contexts > >>> unknown to the currently loaded policy), so leaving the CAP_MAC_ADMIN check > >>> here (versus calling a new security hook here and checking CAP_MAC_ADMIN in > >>> the implementation of that hook for the modules that want that) conflates > >>> two very different things. Prior to this patch, there are no users of > >>> CAP_MAC_ADMIN outside of individual security modules; it is only checked in > >>> module-specific logic within apparmor, safesetid, selinux, and smack, so the > >>> meaning was module-specific. > >> > >> As we had discussed, We do have a security hook as well: > >> > >> https://lore.kernel.org/bpf/20200324180652.GA11855@chromium.org/ > >> > >> The bpf_prog hook which can check for BPF_PROG_TYPE_LSM and implement > >> module specific logic for LSM programs. I thougt that was okay? > >> > >> Kees was in favor of keeping the CAP_MAC_ADMIN check here: > >> > >> https://lore.kernel.org/bpf/202003241133.16C02BE5B@keescook > >> > >> If you feel strongly and Kees agrees, we can remove the CAP_MAC_ADMIN > >> check here, but given that we already have a security hook that meets > >> the requirements, we probably don't need another one. > > > > I would favor removing the CAP_MAC_ADMIN check here, and implementing it in a bpf_prog hook for Smack and AppArmor if they want that. SELinux would implement its own check in its existing bpf_prog hook. > > > The whole notion of one security module calling into another for permission > to do something still gives me the heebee jeebees, but if more nimble minds > than mine think this is a good idea I won't nack it. Well, it's a hook into BPF prog creation, not the BPF LSM specifically, so that's why I think it's general enough control without it being directly weird. :) As far as dropping CAP_MAC_ADMIN, yeah, that should be fine. Creating LSM BPF programs already requires CAP_SYS_ADMIN, so for SELinux-less systems, that's likely fine. If we need to change the BPF program creation access control in the future we can revisit it then.
On 27-Mär 11:59, Kees Cook wrote: > On Fri, Mar 27, 2020 at 09:36:15AM -0700, Casey Schaufler wrote: > > On 3/27/2020 6:43 AM, Stephen Smalley wrote: > > > On 3/27/20 8:41 AM, KP Singh wrote: > > >> On 27-Mär 08:27, Stephen Smalley wrote: > > >>>>> + return -EPERM; [...] > > > > > > I would favor removing the CAP_MAC_ADMIN check here, and implementing it in a bpf_prog hook for Smack and AppArmor if they want that. SELinux would implement its own check in its existing bpf_prog hook. > > > > > The whole notion of one security module calling into another for permission > > to do something still gives me the heebee jeebees, but if more nimble minds > > than mine think this is a good idea I won't nack it. > > Well, it's a hook into BPF prog creation, not the BPF LSM specifically, > so that's why I think it's general enough control without it being > directly weird. :) > > As far as dropping CAP_MAC_ADMIN, yeah, that should be fine. Creating LSM > BPF programs already requires CAP_SYS_ADMIN, so for SELinux-less systems, > that's likely fine. If we need to change the BPF program creation access > control in the future we can revisit it then. Sounds good, I will send out v8 carrying James and Andri's Acks/Review tags, CAP_MAC_ADMIN check removed and some other minor fixes. - KP > > -- > Kees Cook
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h index 83b96895829f..af74712af585 100644 --- a/include/linux/bpf_lsm.h +++ b/include/linux/bpf_lsm.h @@ -17,6 +17,17 @@ #include <linux/lsm_hook_defs.h> #undef LSM_HOOK +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, + const struct bpf_prog *prog); + +#else /* !CONFIG_BPF_LSM */ + +static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, + const struct bpf_prog *prog) +{ + return -EOPNOTSUPP; +} + #endif /* CONFIG_BPF_LSM */ #endif /* _LINUX_BPF_LSM_H */ diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c index e43e29690f22..e6160f0df3f1 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -9,6 +9,8 @@ #include <linux/btf.h> #include <linux/lsm_hooks.h> #include <linux/bpf_lsm.h> +#include <linux/kallsyms.h> +#include <linux/bpf_verifier.h> /* For every LSM hook that allows attachment of BPF programs, declare a nop * function where a BPF program can be attached. @@ -22,6 +24,32 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__) \ #include <linux/lsm_hook_defs.h> #undef LSM_HOOK +#define BPF_LSM_SYM_PREFX "bpf_lsm_" + +int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog, + const struct bpf_prog *prog) +{ + /* Only CAP_MAC_ADMIN users are allowed to make changes to LSM hooks + */ + if (!capable(CAP_MAC_ADMIN)) + return -EPERM; + + if (!prog->gpl_compatible) { + bpf_log(vlog, + "LSM programs must have a GPL compatible license\n"); + return -EINVAL; + } + + if (strncmp(BPF_LSM_SYM_PREFX, prog->aux->attach_func_name, + sizeof(BPF_LSM_SYM_PREFX) - 1)) { + bpf_log(vlog, "attach_btf_id %u points to wrong type name %s\n", + prog->aux->attach_btf_id, prog->aux->attach_func_name); + return -EINVAL; + } + + return 0; +} + const struct bpf_prog_ops lsm_prog_ops = { }; diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 6f397c4da05e..67466dd59a35 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -3710,7 +3710,14 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type, } if (arg == nr_args) { - if (prog->expected_attach_type == BPF_TRACE_FEXIT) { + /* BPF_LSM_MAC programs only have int and void functions they + * can be attached to. When they are attached to a void function + * they result in the creation of an FEXIT trampoline and when + * to a function that returns an int, a MODIFY_RETURN + * trampoline. + */ + if (prog->expected_attach_type == BPF_TRACE_FEXIT || + prog->expected_attach_type == BPF_LSM_MAC) { if (!t) return true; t = btf_type_by_id(btf, t->type); diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 85567a6ea5f9..cea69244c713 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -25,6 +25,7 @@ #include <linux/nospec.h> #include <linux/audit.h> #include <uapi/linux/btf.h> +#include <linux/bpf_lsm.h> #define IS_FD_ARRAY(map) ((map)->map_type == BPF_MAP_TYPE_PERF_EVENT_ARRAY || \ (map)->map_type == BPF_MAP_TYPE_CGROUP_ARRAY || \ @@ -1935,6 +1936,7 @@ bpf_prog_load_check_attach(enum bpf_prog_type prog_type, switch (prog_type) { case BPF_PROG_TYPE_TRACING: + case BPF_PROG_TYPE_LSM: case BPF_PROG_TYPE_STRUCT_OPS: case BPF_PROG_TYPE_EXT: break; @@ -2367,10 +2369,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog) struct file *link_file; int link_fd, err; - if (prog->expected_attach_type != BPF_TRACE_FENTRY && - prog->expected_attach_type != BPF_TRACE_FEXIT && - prog->expected_attach_type != BPF_MODIFY_RETURN && - prog->type != BPF_PROG_TYPE_EXT) { + switch (prog->type) { + case BPF_PROG_TYPE_TRACING: + if (prog->expected_attach_type != BPF_TRACE_FENTRY && + prog->expected_attach_type != BPF_TRACE_FEXIT && + prog->expected_attach_type != BPF_MODIFY_RETURN) { + err = -EINVAL; + goto out_put_prog; + } + break; + case BPF_PROG_TYPE_EXT: + if (prog->expected_attach_type != 0) { + err = -EINVAL; + goto out_put_prog; + } + break; + case BPF_PROG_TYPE_LSM: + if (prog->expected_attach_type != BPF_LSM_MAC) { + err = -EINVAL; + goto out_put_prog; + } + break; + default: err = -EINVAL; goto out_put_prog; } @@ -2449,16 +2469,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) if (IS_ERR(prog)) return PTR_ERR(prog); - if (prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT && - prog->type != BPF_PROG_TYPE_TRACING && - prog->type != BPF_PROG_TYPE_EXT && - prog->type != BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE) { - err = -EINVAL; - goto out_put_prog; - } - - if (prog->type == BPF_PROG_TYPE_TRACING || - prog->type == BPF_PROG_TYPE_EXT) { + switch (prog->type) { + case BPF_PROG_TYPE_TRACING: + case BPF_PROG_TYPE_EXT: + case BPF_PROG_TYPE_LSM: if (attr->raw_tracepoint.name) { /* The attach point for this category of programs * should be specified via btf_id during program load. @@ -2466,11 +2480,14 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) err = -EINVAL; goto out_put_prog; } - if (prog->expected_attach_type == BPF_TRACE_RAW_TP) + if (prog->type == BPF_PROG_TYPE_TRACING && + prog->expected_attach_type == BPF_TRACE_RAW_TP) { tp_name = prog->aux->attach_func_name; - else - return bpf_tracing_prog_attach(prog); - } else { + break; + } + return bpf_tracing_prog_attach(prog); + case BPF_PROG_TYPE_RAW_TRACEPOINT: + case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE: if (strncpy_from_user(buf, u64_to_user_ptr(attr->raw_tracepoint.name), sizeof(buf) - 1) < 0) { @@ -2479,6 +2496,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr) } buf[sizeof(buf) - 1] = 0; tp_name = buf; + break; + default: + err = -EINVAL; + goto out_put_prog; } btp = bpf_get_raw_tracepoint(tp_name); diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c index f30bca2a4d01..9be85aa4ec5f 100644 --- a/kernel/bpf/trampoline.c +++ b/kernel/bpf/trampoline.c @@ -6,6 +6,7 @@ #include <linux/ftrace.h> #include <linux/rbtree_latch.h> #include <linux/perf_event.h> +#include <linux/btf.h> /* dummy _ops. The verifier will operate on target program's ops. */ const struct bpf_verifier_ops bpf_extension_verifier_ops = { @@ -233,15 +234,23 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr) return err; } -static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(enum bpf_attach_type t) +static enum bpf_tramp_prog_type bpf_attach_type_to_tramp(struct bpf_prog *prog) { - switch (t) { + switch (prog->expected_attach_type) { case BPF_TRACE_FENTRY: return BPF_TRAMP_FENTRY; case BPF_MODIFY_RETURN: return BPF_TRAMP_MODIFY_RETURN; case BPF_TRACE_FEXIT: return BPF_TRAMP_FEXIT; + case BPF_LSM_MAC: + if (!prog->aux->attach_func_proto->type) + /* The function returns void, we cannot modify its + * return value. + */ + return BPF_TRAMP_FEXIT; + else + return BPF_TRAMP_MODIFY_RETURN; default: return BPF_TRAMP_REPLACE; } @@ -255,7 +264,7 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog) int cnt; tr = prog->aux->trampoline; - kind = bpf_attach_type_to_tramp(prog->expected_attach_type); + kind = bpf_attach_type_to_tramp(prog); mutex_lock(&tr->mutex); if (tr->extension_prog) { /* cannot attach fentry/fexit if extension prog is attached. @@ -305,7 +314,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog) int err; tr = prog->aux->trampoline; - kind = bpf_attach_type_to_tramp(prog->expected_attach_type); + kind = bpf_attach_type_to_tramp(prog); mutex_lock(&tr->mutex); if (kind == BPF_TRAMP_REPLACE) { WARN_ON_ONCE(!tr->extension_prog); diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2ea2a868324e..8808c06f2571 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -20,6 +20,7 @@ #include <linux/perf_event.h> #include <linux/ctype.h> #include <linux/error-injection.h> +#include <linux/bpf_lsm.h> #include "disasm.h" @@ -6488,8 +6489,9 @@ static int check_return_code(struct bpf_verifier_env *env) struct tnum range = tnum_range(0, 1); int err; - /* The struct_ops func-ptr's return type could be "void" */ - if (env->prog->type == BPF_PROG_TYPE_STRUCT_OPS && + /* LSM and struct_ops func-ptr's return type could be "void" */ + if ((env->prog->type == BPF_PROG_TYPE_STRUCT_OPS || + env->prog->type == BPF_PROG_TYPE_LSM) && !prog->aux->attach_func_proto->type) return 0; @@ -9919,7 +9921,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) return check_struct_ops_btf_id(env); - if (prog->type != BPF_PROG_TYPE_TRACING && !prog_extension) + if (prog->type != BPF_PROG_TYPE_TRACING && + prog->type != BPF_PROG_TYPE_LSM && + !prog_extension) return 0; if (!btf_id) { @@ -10050,8 +10054,16 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) return -EINVAL; /* fallthrough */ case BPF_MODIFY_RETURN: + case BPF_LSM_MAC: case BPF_TRACE_FENTRY: case BPF_TRACE_FEXIT: + prog->aux->attach_func_name = tname; + if (prog->type == BPF_PROG_TYPE_LSM) { + ret = bpf_lsm_verify_prog(&env->log, prog); + if (ret < 0) + return ret; + } + if (!btf_type_is_func(t)) { verbose(env, "attach_btf_id %u is not a function\n", btf_id); @@ -10066,7 +10078,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env) tr = bpf_trampoline_lookup(key); if (!tr) return -ENOMEM; - prog->aux->attach_func_name = tname; /* t is either vmlinux type or another program's type */ prog->aux->attach_func_proto = t; mutex_lock(&tr->mutex);