From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chenbo Feng Subject: Re: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations Date: Tue, 10 Oct 2017 10:54:50 -0700 Message-ID: References: <20171009222028.13096-1-chenbofeng.kernel@gmail.com> <20171009222028.13096-5-chenbofeng.kernel@gmail.com> <1507645097.30616.6.camel@tycho.nsa.gov> <1507647148.30616.14.camel@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: Chenbo Feng , linux-security-module@vger.kernel.org, netdev@vger.kernel.org, SELinux , Alexei Starovoitov , Daniel Borkmann , Lorenzo Colitti To: Stephen Smalley Return-path: Received: from mail-lf0-f52.google.com ([209.85.215.52]:43400 "EHLO mail-lf0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755794AbdJJRyx (ORCPT ); Tue, 10 Oct 2017 13:54:53 -0400 Received: by mail-lf0-f52.google.com with SMTP id a16so14552521lfk.0 for ; Tue, 10 Oct 2017 10:54:52 -0700 (PDT) In-Reply-To: <1507647148.30616.14.camel@tycho.nsa.gov> Sender: netdev-owner@vger.kernel.org List-ID: On Tue, Oct 10, 2017 at 7:52 AM, Stephen Smalley wrote: > On Tue, 2017-10-10 at 10:18 -0400, Stephen Smalley wrote: >> On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote: >> > From: Chenbo Feng >> > >> > Implement the actual checks introduced to eBPF related syscalls. >> > This >> > implementation use the security field inside bpf object to store a >> > sid that >> > identify the bpf object. And when processes try to access the >> > object, >> > selinux will check if processes have the right privileges. The >> > creation >> > of eBPF object are also checked at the general bpf check hook and >> > new >> > cmd introduced to eBPF domain can also be checked there. >> > >> > Signed-off-by: Chenbo Feng >> > Acked-by: Alexei Starovoitov >> > --- >> > security/selinux/hooks.c | 111 >> > ++++++++++++++++++++++++++++++++++++ >> > security/selinux/include/classmap.h | 2 + >> > security/selinux/include/objsec.h | 4 ++ >> > 3 files changed, 117 insertions(+) >> > >> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> > index f5d304736852..41aba4e3d57c 100644 >> > --- a/security/selinux/hooks.c >> > +++ b/security/selinux/hooks.c >> > @@ -85,6 +85,7 @@ >> > #include >> > #include >> > #include >> > +#include >> > >> > #include "avc.h" >> > #include "objsec.h" >> > @@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void >> > *ib_sec) >> > } >> > #endif >> > >> > +#ifdef CONFIG_BPF_SYSCALL >> > +static int selinux_bpf(int cmd, union bpf_attr *attr, >> > + unsigned int size) >> > +{ >> > + u32 sid = current_sid(); >> > + int ret; >> > + >> > + switch (cmd) { >> > + case BPF_MAP_CREATE: >> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_MAP, >> > BPF_MAP__CREATE, >> > + NULL); >> > + break; >> > + case BPF_PROG_LOAD: >> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG, >> > BPF_PROG__LOAD, >> > + NULL); >> > + break; >> > + default: >> > + ret = 0; >> > + break; >> > + } >> > + >> > + return ret; >> > +} >> > + >> > +static u32 bpf_map_fmode_to_av(fmode_t fmode) >> > +{ >> > + u32 av = 0; >> > + >> > + if (f_mode & FMODE_READ) >> > + av |= BPF_MAP__READ; >> > + if (f_mode & FMODE_WRITE) >> > + av |= BPF_MAP__WRITE; >> > + return av; >> > +} >> > + >> > +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) >> > +{ >> > + u32 sid = current_sid(); >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = map->security; >> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP, >> > + bpf_map_fmode_to_av(fmode), NULL); >> > +} >> > + >> > +static int selinux_bpf_prog(struct bpf_prog *prog) >> > +{ >> > + u32 sid = current_sid(); >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = prog->aux->security; >> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG, >> > + BPF_PROG__USE, NULL); >> > +} >> > + >> > +static int selinux_bpf_map_alloc(struct bpf_map *map) >> > +{ >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL); >> > + if (!bpfsec) >> > + return -ENOMEM; >> > + >> > + bpfsec->sid = current_sid(); >> > + map->security = bpfsec; >> > + >> > + return 0; >> > +} >> > + >> > +static void selinux_bpf_map_free(struct bpf_map *map) >> > +{ >> > + struct bpf_security_struct *bpfsec = map->security; >> > + >> > + map->security = NULL; >> > + kfree(bpfsec); >> > +} >> > + >> > +static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux) >> > +{ >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL); >> > + if (!bpfsec) >> > + return -ENOMEM; >> > + >> > + bpfsec->sid = current_sid(); >> > + aux->security = bpfsec; >> > + >> > + return 0; >> > +} >> > + >> > +static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) >> > +{ >> > + struct bpf_security_struct *bpfsec = aux->security; >> > + >> > + aux->security = NULL; >> > + kfree(bpfsec); >> > +} >> > +#endif >> > + >> > static struct security_hook_list selinux_hooks[] >> > __lsm_ro_after_init >> > = { >> > LSM_HOOK_INIT(binder_set_context_mgr, >> > selinux_binder_set_context_mgr), >> > LSM_HOOK_INIT(binder_transaction, >> > selinux_binder_transaction), >> > @@ -6471,6 +6572,16 @@ static struct security_hook_list >> > selinux_hooks[] __lsm_ro_after_init = { >> > LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match), >> > LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free), >> > #endif >> > + >> > +#ifdef CONFIG_BPF_SYSCALL >> > + LSM_HOOK_INIT(bpf, selinux_bpf), >> > + LSM_HOOK_INIT(bpf_map, selinux_bpf_map), >> > + LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog), >> > + LSM_HOOK_INIT(bpf_map_alloc_security, >> > selinux_bpf_map_alloc), >> > + LSM_HOOK_INIT(bpf_prog_alloc_security, >> > selinux_bpf_prog_alloc), >> > + LSM_HOOK_INIT(bpf_map_free_security, >> > selinux_bpf_map_free), >> > + LSM_HOOK_INIT(bpf_prog_free_security, >> > selinux_bpf_prog_free), >> > +#endif >> > }; >> > >> > static __init int selinux_init(void) >> > diff --git a/security/selinux/include/classmap.h >> > b/security/selinux/include/classmap.h >> > index 35ffb29a69cb..7253c5eea59c 100644 >> > --- a/security/selinux/include/classmap.h >> > +++ b/security/selinux/include/classmap.h >> > @@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = >> > { >> > { "access", NULL } }, >> > { "infiniband_endport", >> > { "manage_subnet", NULL } }, >> > + { "bpf_map", {"create", "read", "write"} }, >> > + { "bpf_prog", {"load", "use"} }, >> >> Again I have to ask: do you truly need/want two separate classes, or >> would a single class with distinct permissions suffice, ala: >> { "bpf", { "create_map", "read_map", "write_map", >> "prog_load", >> "prog_use" } }, >> >> and then allow A self:bpf { create_map read_map write_map prog_load >> prog_use }; would be stored in a single policy avtab rule, and be >> cached in a single AVC entry. > Sorry I missed to reply on this, I keep it that way because sometimes we need to grant the permission of accessing eBPF maps and programs separately. But keep them in a single class definitely works for me. > I guess for consistency in naming it should be either: > { "bpf", { "map_create", "map_read", "map_write", "prog_load", > "prog_use" } }, > > or: > > { "bpf", { "create_map", "read_map", "write_map", "load_prog", > "use_prog" } }, > > >> > { NULL } >> > }; >> > >> > diff --git a/security/selinux/include/objsec.h >> > b/security/selinux/include/objsec.h >> > index 1649cd18eb0b..3d54468ce334 100644 >> > --- a/security/selinux/include/objsec.h >> > +++ b/security/selinux/include/objsec.h >> > @@ -150,6 +150,10 @@ struct pkey_security_struct { >> > u32 sid; /* SID of pkey */ >> > }; >> > >> > +struct bpf_security_struct { >> > + u32 sid; /*SID of bpf obj creater*/ >> > +}; >> > + >> > extern unsigned int selinux_checkreqprot; >> > >> > #endif /* _SELINUX_OBJSEC_H_ */ From mboxrd@z Thu Jan 1 00:00:00 1970 From: fengc@google.com (Chenbo Feng) Date: Tue, 10 Oct 2017 10:54:50 -0700 Subject: [PATCH net-next v2 4/5] selinux: bpf: Add selinux check for eBPF syscall operations In-Reply-To: <1507647148.30616.14.camel@tycho.nsa.gov> References: <20171009222028.13096-1-chenbofeng.kernel@gmail.com> <20171009222028.13096-5-chenbofeng.kernel@gmail.com> <1507645097.30616.6.camel@tycho.nsa.gov> <1507647148.30616.14.camel@tycho.nsa.gov> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Tue, Oct 10, 2017 at 7:52 AM, Stephen Smalley wrote: > On Tue, 2017-10-10 at 10:18 -0400, Stephen Smalley wrote: >> On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote: >> > From: Chenbo Feng >> > >> > Implement the actual checks introduced to eBPF related syscalls. >> > This >> > implementation use the security field inside bpf object to store a >> > sid that >> > identify the bpf object. And when processes try to access the >> > object, >> > selinux will check if processes have the right privileges. The >> > creation >> > of eBPF object are also checked at the general bpf check hook and >> > new >> > cmd introduced to eBPF domain can also be checked there. >> > >> > Signed-off-by: Chenbo Feng >> > Acked-by: Alexei Starovoitov >> > --- >> > security/selinux/hooks.c | 111 >> > ++++++++++++++++++++++++++++++++++++ >> > security/selinux/include/classmap.h | 2 + >> > security/selinux/include/objsec.h | 4 ++ >> > 3 files changed, 117 insertions(+) >> > >> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >> > index f5d304736852..41aba4e3d57c 100644 >> > --- a/security/selinux/hooks.c >> > +++ b/security/selinux/hooks.c >> > @@ -85,6 +85,7 @@ >> > #include >> > #include >> > #include >> > +#include >> > >> > #include "avc.h" >> > #include "objsec.h" >> > @@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void >> > *ib_sec) >> > } >> > #endif >> > >> > +#ifdef CONFIG_BPF_SYSCALL >> > +static int selinux_bpf(int cmd, union bpf_attr *attr, >> > + unsigned int size) >> > +{ >> > + u32 sid = current_sid(); >> > + int ret; >> > + >> > + switch (cmd) { >> > + case BPF_MAP_CREATE: >> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_MAP, >> > BPF_MAP__CREATE, >> > + NULL); >> > + break; >> > + case BPF_PROG_LOAD: >> > + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG, >> > BPF_PROG__LOAD, >> > + NULL); >> > + break; >> > + default: >> > + ret = 0; >> > + break; >> > + } >> > + >> > + return ret; >> > +} >> > + >> > +static u32 bpf_map_fmode_to_av(fmode_t fmode) >> > +{ >> > + u32 av = 0; >> > + >> > + if (f_mode & FMODE_READ) >> > + av |= BPF_MAP__READ; >> > + if (f_mode & FMODE_WRITE) >> > + av |= BPF_MAP__WRITE; >> > + return av; >> > +} >> > + >> > +static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) >> > +{ >> > + u32 sid = current_sid(); >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = map->security; >> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP, >> > + bpf_map_fmode_to_av(fmode), NULL); >> > +} >> > + >> > +static int selinux_bpf_prog(struct bpf_prog *prog) >> > +{ >> > + u32 sid = current_sid(); >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = prog->aux->security; >> > + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG, >> > + BPF_PROG__USE, NULL); >> > +} >> > + >> > +static int selinux_bpf_map_alloc(struct bpf_map *map) >> > +{ >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL); >> > + if (!bpfsec) >> > + return -ENOMEM; >> > + >> > + bpfsec->sid = current_sid(); >> > + map->security = bpfsec; >> > + >> > + return 0; >> > +} >> > + >> > +static void selinux_bpf_map_free(struct bpf_map *map) >> > +{ >> > + struct bpf_security_struct *bpfsec = map->security; >> > + >> > + map->security = NULL; >> > + kfree(bpfsec); >> > +} >> > + >> > +static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux) >> > +{ >> > + struct bpf_security_struct *bpfsec; >> > + >> > + bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL); >> > + if (!bpfsec) >> > + return -ENOMEM; >> > + >> > + bpfsec->sid = current_sid(); >> > + aux->security = bpfsec; >> > + >> > + return 0; >> > +} >> > + >> > +static void selinux_bpf_prog_free(struct bpf_prog_aux *aux) >> > +{ >> > + struct bpf_security_struct *bpfsec = aux->security; >> > + >> > + aux->security = NULL; >> > + kfree(bpfsec); >> > +} >> > +#endif >> > + >> > static struct security_hook_list selinux_hooks[] >> > __lsm_ro_after_init >> > = { >> > LSM_HOOK_INIT(binder_set_context_mgr, >> > selinux_binder_set_context_mgr), >> > LSM_HOOK_INIT(binder_transaction, >> > selinux_binder_transaction), >> > @@ -6471,6 +6572,16 @@ static struct security_hook_list >> > selinux_hooks[] __lsm_ro_after_init = { >> > LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match), >> > LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free), >> > #endif >> > + >> > +#ifdef CONFIG_BPF_SYSCALL >> > + LSM_HOOK_INIT(bpf, selinux_bpf), >> > + LSM_HOOK_INIT(bpf_map, selinux_bpf_map), >> > + LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog), >> > + LSM_HOOK_INIT(bpf_map_alloc_security, >> > selinux_bpf_map_alloc), >> > + LSM_HOOK_INIT(bpf_prog_alloc_security, >> > selinux_bpf_prog_alloc), >> > + LSM_HOOK_INIT(bpf_map_free_security, >> > selinux_bpf_map_free), >> > + LSM_HOOK_INIT(bpf_prog_free_security, >> > selinux_bpf_prog_free), >> > +#endif >> > }; >> > >> > static __init int selinux_init(void) >> > diff --git a/security/selinux/include/classmap.h >> > b/security/selinux/include/classmap.h >> > index 35ffb29a69cb..7253c5eea59c 100644 >> > --- a/security/selinux/include/classmap.h >> > +++ b/security/selinux/include/classmap.h >> > @@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = >> > { >> > { "access", NULL } }, >> > { "infiniband_endport", >> > { "manage_subnet", NULL } }, >> > + { "bpf_map", {"create", "read", "write"} }, >> > + { "bpf_prog", {"load", "use"} }, >> >> Again I have to ask: do you truly need/want two separate classes, or >> would a single class with distinct permissions suffice, ala: >> { "bpf", { "create_map", "read_map", "write_map", >> "prog_load", >> "prog_use" } }, >> >> and then allow A self:bpf { create_map read_map write_map prog_load >> prog_use }; would be stored in a single policy avtab rule, and be >> cached in a single AVC entry. > Sorry I missed to reply on this, I keep it that way because sometimes we need to grant the permission of accessing eBPF maps and programs separately. But keep them in a single class definitely works for me. > I guess for consistency in naming it should be either: > { "bpf", { "map_create", "map_read", "map_write", "prog_load", > "prog_use" } }, > > or: > > { "bpf", { "create_map", "read_map", "write_map", "load_prog", > "use_prog" } }, > > >> > { NULL } >> > }; >> > >> > diff --git a/security/selinux/include/objsec.h >> > b/security/selinux/include/objsec.h >> > index 1649cd18eb0b..3d54468ce334 100644 >> > --- a/security/selinux/include/objsec.h >> > +++ b/security/selinux/include/objsec.h >> > @@ -150,6 +150,10 @@ struct pkey_security_struct { >> > u32 sid; /* SID of pkey */ >> > }; >> > >> > +struct bpf_security_struct { >> > + u32 sid; /*SID of bpf obj creater*/ >> > +}; >> > + >> > extern unsigned int selinux_checkreqprot; >> > >> > #endif /* _SELINUX_OBJSEC_H_ */ -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html