From mboxrd@z Thu Jan 1 00:00:00 1970 From: sds@tycho.nsa.gov (Stephen Smalley) Date: Thu, 07 Sep 2017 08:32:19 -0400 Subject: [PATCH 1/3] security: bpf: Add eBPF LSM hooks to security module In-Reply-To: References: <20170831205635.80256-1-chenbofeng.kernel@gmail.com> <20170831205635.80256-2-chenbofeng.kernel@gmail.com> <1504270223.8240.2.camel@tycho.nsa.gov> Message-ID: <1504787539.22845.1.camel@tycho.nsa.gov> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Tue, 2017-09-05 at 15:24 -0700, Chenbo Feng via Selinux wrote: > On Fri, Sep 1, 2017 at 5:50 AM, Stephen Smalley > wrote: > > On Thu, 2017-08-31 at 13:56 -0700, Chenbo Feng wrote: > > > From: Chenbo Feng > > > > > > Introduce 5 LSM hooks to provide finer granularity controls on > > > eBPF > > > related operations including create eBPF maps, modify and read > > > eBPF > > > maps > > > content and load eBPF programs to the kernel. Hooks use the new > > > security > > > pointer inside the eBPF map struct to store the owner's security > > > information and the different security modules can perform > > > different > > > checks based on the information stored inside the security field. > > > > > > Signed-off-by: Chenbo Feng > > > --- > > > ?include/linux/lsm_hooks.h | 41 > > > +++++++++++++++++++++++++++++++++++++++++ > > > ?include/linux/security.h??| 36 > > > ++++++++++++++++++++++++++++++++++++ > > > ?security/security.c???????| 28 ++++++++++++++++++++++++++++ > > > ?3 files changed, 105 insertions(+) > > > > > > diff --git a/include/linux/lsm_hooks.h > > > b/include/linux/lsm_hooks.h > > > index ce02f76a6188..3aaf9a08a983 100644 > > > --- a/include/linux/lsm_hooks.h > > > +++ b/include/linux/lsm_hooks.h > > > @@ -1353,6 +1353,32 @@ > > > ? *???@inode we wish to get the security context of. > > > ? *???@ctx is a pointer in which to place the allocated security > > > context. > > > ? *???@ctxlen points to the place to put the length of @ctx. > > > + * > > > + * Security hooks for using the eBPF maps and programs > > > functionalities through > > > + * eBPF syscalls. > > > + * > > > + * @bpf_map_create: > > > + *???Check permissions prior to creating a new bpf map. > > > + *???Return 0 if the permission is granted. > > > + * > > > + * @bpf_map_modify: > > > + *???Check permission prior to insert, update and delete map > > > content. > > > + *???@map pointer to the struct bpf_map that contains map > > > information. > > > + *???Return 0 if the permission is granted. > > > + * > > > + * @bpf_map_read: > > > + *???Check permission prior to read a bpf map content. > > > + *???@map pointer to the struct bpf_map that contains map > > > information. > > > + *???Return 0 if the permission is granted. > > > + * > > > + * @bpf_prog_load: > > > + *???Check permission prior to load eBPF program. > > > + *???Return 0 if the permission is granted. > > > + * > > > + * @bpf_post_create: > > > + *???Initialize the bpf object security field inside struct > > > bpf_maps and > > > + *???it is used for future security checks. > > > + * > > > ? */ > > > ?union security_list_options { > > > ??????int (*binder_set_context_mgr)(struct task_struct *mgr); > > > @@ -1685,6 +1711,14 @@ union security_list_options { > > > ??????????????????????????????struct audit_context *actx); > > > ??????void (*audit_rule_free)(void *lsmrule); > > > ?#endif /* CONFIG_AUDIT */ > > > + > > > +#ifdef CONFIG_BPF_SYSCALL > > > +?????int (*bpf_map_create)(void); > > > +?????int (*bpf_map_read)(struct bpf_map *map); > > > +?????int (*bpf_map_modify)(struct bpf_map *map); > > > +?????int (*bpf_prog_load)(void); > > > +?????int (*bpf_post_create)(struct bpf_map *map); > > > +#endif /* CONFIG_BPF_SYSCALL */ > > > ?}; > > > > > > ?struct security_hook_heads { > > > @@ -1905,6 +1939,13 @@ struct security_hook_heads { > > > ??????struct list_head audit_rule_match; > > > ??????struct list_head audit_rule_free; > > > ?#endif /* CONFIG_AUDIT */ > > > +#ifdef CONFIG_BPF_SYSCALL > > > +?????struct list_head bpf_map_create; > > > +?????struct list_head bpf_map_read; > > > +?????struct list_head bpf_map_modify; > > > +?????struct list_head bpf_prog_load; > > > +?????struct list_head bpf_post_create; > > > +#endif /* CONFIG_BPF_SYSCALL */ > > > ?} __randomize_layout; > > > > > > ?/* > > > diff --git a/include/linux/security.h b/include/linux/security.h > > > index 458e24bea2d4..0656a4f74d14 100644 > > > --- a/include/linux/security.h > > > +++ b/include/linux/security.h > > > @@ -31,6 +31,7 @@ > > > ?#include > > > ?#include > > > ?#include > > > +#include > > > > > > ?struct linux_binprm; > > > ?struct cred; > > > @@ -1735,6 +1736,41 @@ static inline void > > > securityfs_remove(struct > > > dentry *dentry) > > > > > > ?#endif > > > > > > +#ifdef CONFIG_BPF_SYSCALL > > > +#ifdef CONFIG_SECURITY > > > +int security_map_create(void); > > > +int security_map_modify(struct bpf_map *map); > > > +int security_map_read(struct bpf_map *map); > > > +int security_prog_load(void); > > > +int security_post_create(struct bpf_map *map); > > > +#else > > > +static inline int security_map_create(void) > > > +{ > > > +?????return 0; > > > +} > > > + > > > +static inline int security_map_read(struct bpf_map *map) > > > +{ > > > +?????return 0; > > > +} > > > + > > > +static inline int security_map_modify(struct bpf_map *map) > > > +{ > > > +?????return 0; > > > +} > > > + > > > +static inline int security_prog_load(void) > > > +{ > > > +?????return 0; > > > +} > > > + > > > +static inline int security_post_create(struct bpf_map *map) > > > +{ > > > +?????return 0; > > > +} > > > +#endif /* CONFIG_SECURITY */ > > > +#endif /* CONFIG_BPF_SYSCALL */ > > > > These should be named consistently with the ones in lsm_hooks.h and > > should unambiguously indicate that these are hooks for bpf > > objects/operations, i.e. security_bpf_map_create(), > > security_bpf_map_read(), etc. > > > > Thanks for pointing out, will fix this. > > Do you need this level of granularity? > > > > The cover letter of this patch series described a possible use cases > of > these lsm hooks and this level of granularity would be ideal to reach > that > goal. We can also implement two hooks such as bpf_obj_create and > bpf_obj_use to restrict the creation and using when get the bpf fd > from > kernel. But that will be less powerful and flexible. > > Could you coalesce the map_create() and post_map_create() hooks > > into > > one hook and just unwind the create in that case? > > > > Okay, I will take a look on how to fix this. Also, what you called security_post_create() would normally be called something like security_bpf_alloc_security(), and would have a corresponding security_bpf_free_security() hook too. However, whether or not you still need this security field and hook at all is unclear to me, given the direction the discussion has gone. > > Why do you label bpf maps but not bpf progs???Should we be > > controlling > > the ability to attach/detach a bpf prog (partly controlled by > > CAP_NET_ADMIN, but also somewhat broad in scope and doesn't allow > > control based on who created the prog)? > > > > Should there be a top-level security_bpf_use() hook and permission > > check that limits ability to use bpf() at all? > > > > This could be useful but having additional lsm hooks check when > reading > and write to eBPF maps may cause performance issue. Instead maybe we > could have a hook for creating eBPF object and retrieve object fd to > restrict > the access. > > > + > > > ?#ifdef CONFIG_SECURITY > > > > > > ?static inline char *alloc_secdata(void) > > > diff --git a/security/security.c b/security/security.c > > > index 55b5997e4b72..02272f93a89e 100644 > > > --- a/security/security.c > > > +++ b/security/security.c > > > @@ -12,6 +12,7 @@ > > > ? *???(at your option) any later version. > > > ? */ > > > > > > +#include > > > ?#include > > > ?#include > > > ?#include > > > @@ -1708,3 +1709,30 @@ int security_audit_rule_match(u32 secid, > > > u32 > > > field, u32 op, void *lsmrule, > > > ??????????????????????????????actx); > > > ?} > > > ?#endif /* CONFIG_AUDIT */ > > > + > > > +#ifdef CONFIG_BPF_SYSCALL > > > +int security_map_create(void) > > > +{ > > > +?????return call_int_hook(bpf_map_create, 0); > > > +} > > > + > > > +int security_map_modify(struct bpf_map *map) > > > +{ > > > +?????return call_int_hook(bpf_map_modify, 0, map); > > > +} > > > + > > > +int security_map_read(struct bpf_map *map) > > > +{ > > > +?????return call_int_hook(bpf_map_read, 0, map); > > > +} > > > + > > > +int security_prog_load(void) > > > +{ > > > +?????return call_int_hook(bpf_prog_load, 0); > > > +} > > > + > > > +int security_post_create(struct bpf_map *map) > > > +{ > > > +?????return call_int_hook(bpf_post_create, 0, map); > > > +} > > > +#endif /* CONFIG_BPF_SYSCALL */ -- 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