diff mbox

[net-next,v2,4/5] selinux: bpf: Add selinux check for eBPF syscall operations

Message ID 20171009222028.13096-5-chenbofeng.kernel@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chenbo Feng Oct. 9, 2017, 10:20 p.m. UTC 
From: Chenbo Feng <fengc@google.com>

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 <fengc@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 security/selinux/hooks.c            | 111 ++++++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h |   2 +
 security/selinux/include/objsec.h   |   4 ++
 3 files changed, 117 insertions(+)

Comments

Stephen Smalley Oct. 10, 2017, 2:18 p.m. UTC | #1 
On Mon, 2017-10-09 at 15:20 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
> 
> 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 <fengc@google.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  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 <linux/export.h>
>  #include <linux/msg.h>
>  #include <linux/shm.h>
> +#include <linux/bpf.h>
>  
>  #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.

>  	{ 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Smalley Oct. 10, 2017, 2:52 p.m. UTC | #2 
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 <fengc@google.com>
> > 
> > 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 <fengc@google.com>
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> >  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 <linux/export.h>
> >  #include <linux/msg.h>
> >  #include <linux/shm.h>
> > +#include <linux/bpf.h>
> >  
> >  #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.

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chenbo Feng Oct. 10, 2017, 5:54 p.m. UTC | #3 
On Tue, Oct 10, 2017 at 7:52 AM, Stephen Smalley <sds@tycho.nsa.gov> 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 <fengc@google.com>
>> >
>> > 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 <fengc@google.com>
>> > Acked-by: Alexei Starovoitov <ast@kernel.org>
>> > ---
>> >  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 <linux/export.h>
>> >  #include <linux/msg.h>
>> >  #include <linux/shm.h>
>> > +#include <linux/bpf.h>
>> >
>> >  #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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
kernel test robot Oct. 10, 2017, 5:59 p.m. UTC | #4 
Hi Chenbo,

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Chenbo-Feng/bpf-security-New-file-mode-and-LSM-hooks-for-eBPF-object-permission-control/20171011-010349
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   security//selinux/hooks.c: In function 'bpf_map_fmode_to_av':
>> security//selinux/hooks.c:6284:6: error: 'f_mode' undeclared (first use in this function)
     if (f_mode & FMODE_READ)
         ^
   security//selinux/hooks.c:6284:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/f_mode +6284 security//selinux/hooks.c

  6279	
  6280	static u32 bpf_map_fmode_to_av(fmode_t fmode)
  6281	{
  6282		u32 av = 0;
  6283	
> 6284		if (f_mode & FMODE_READ)
  6285			av |= BPF_MAP__READ;
  6286		if (f_mode & FMODE_WRITE)
  6287			av |= BPF_MAP__WRITE;
  6288		return av;
  6289	}
  6290	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
kernel test robot Oct. 10, 2017, 9:30 p.m. UTC | #5 
Hi Chenbo,

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Chenbo-Feng/bpf-security-New-file-mode-and-LSM-hooks-for-eBPF-object-permission-control/20171011-010349
config: x86_64-randconfig-u0-10110310 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/init.h:4:0,
                    from security/selinux/hooks.c:27:
   security/selinux/hooks.c: In function 'bpf_map_fmode_to_av':
   security/selinux/hooks.c:6284:6: error: 'f_mode' undeclared (first use in this function)
     if (f_mode & FMODE_READ)
         ^
   include/linux/compiler.h:156:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> security/selinux/hooks.c:6284:2: note: in expansion of macro 'if'
     if (f_mode & FMODE_READ)
     ^~
   security/selinux/hooks.c:6284:6: note: each undeclared identifier is reported only once for each function it appears in
     if (f_mode & FMODE_READ)
         ^
   include/linux/compiler.h:156:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> security/selinux/hooks.c:6284:2: note: in expansion of macro 'if'
     if (f_mode & FMODE_READ)
     ^~

vim +/if +6284 security/selinux/hooks.c

  6279	
  6280	static u32 bpf_map_fmode_to_av(fmode_t fmode)
  6281	{
  6282		u32 av = 0;
  6283	
> 6284		if (f_mode & FMODE_READ)
  6285			av |= BPF_MAP__READ;
  6286		if (f_mode & FMODE_WRITE)
  6287			av |= BPF_MAP__WRITE;
  6288		return av;
  6289	}
  6290	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Stephen Smalley Oct. 11, 2017, 1 p.m. UTC | #6 
On Tue, 2017-10-10 at 10:54 -0700, Chenbo Feng via Selinux wrote:
> On Tue, Oct 10, 2017 at 7:52 AM, Stephen Smalley <sds@tycho.nsa.gov>
> 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 <fengc@google.com>
> > > > 
> > > > 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 <fengc@google.com>
> > > > Acked-by: Alexei Starovoitov <ast@kernel.org>
> > > > ---
> > > >  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 <linux/export.h>
> > > >  #include <linux/msg.h>
> > > >  #include <linux/shm.h>
> > > > +#include <linux/bpf.h>
> > > > 
> > > >  #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.

If we anticipated a large number of permissions being defined for
either bpf maps or programs, or if we were labeling them differently
(e.g. inheriting from creator for one, while using type transitions for
another), then it might make more sense to split the class (so that we
can support up to 32 distinct permissions for each one, or because we'd
never end up allowing both map and prog permissions to the same target
SID/context).  But in this case I can't see any benefit to using two
classes, and it would consume more memory to do so.

BTW, please be sure to cc Paul Moore on these patches if you aren't
already since he is the selinux kernel tree maintainer.

> > 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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 <linux/export.h>
 #include <linux/msg.h>
 #include <linux/shm.h>
+#include <linux/bpf.h>
 
 #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"} },
 	{ 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_ */