| Message ID | 20200220175250.10795-5-kpsingh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MAC and Audit policy using eBPF (KRSI) | expand |
On 2/20/2020 9:52 AM, KP Singh wrote: > From: KP Singh <kpsingh@google.com> Again, sorry for trimming the CC list, but thunderbird ... > > Each LSM hook defines a static key i.e. bpf_lsm_<name> > and a bpf_lsm_<name>_set_enabled function to toggle the key > which enables/disables the branch which executes the BPF programs > attached to the LSM hook. > > Use of static keys was suggested in upstream discussion: > > https://lore.kernel.org/bpf/1cd10710-a81b-8f9b-696d-aa40b0a67225@iogearbox.net/ > > and results in the following assembly: > > 0x0000000000001e31 <+65>: jmpq 0x1e36 <security_bprm_check+70> > 0x0000000000001e36 <+70>: nopl 0x0(%rax,%rax,1) > 0x0000000000001e3b <+75>: xor %eax,%eax > 0x0000000000001e3d <+77>: jmp 0x1e25 <security_bprm_check+53> > > which avoids an indirect branch and results in lower overhead which is > especially helpful for LSM hooks in performance hotpaths. > > Given the ability to toggle the BPF trampolines, some hooks which do > not call call_<int, void>_hooks as they have different default return > values, also gain support for BPF program attachment. > > There are some hooks like security_setprocattr and security_getprocattr > which are not instrumentable as they do not provide any monitoring or > access control decisions. If required, generation of BTF type > information for these hooks can be also be blacklisted. > > Signed-off-by: KP Singh <kpsingh@google.com> > --- > include/linux/bpf_lsm.h | 30 +++++++++++++++++++++++++++--- > kernel/bpf/bpf_lsm.c | 28 ++++++++++++++++++++++++++++ > security/security.c | 32 ++++++++++++++++++++++++++++++++ > 3 files changed, 87 insertions(+), 3 deletions(-) > > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h > index f867f72f6aa9..53dcda8ace01 100644 > --- a/include/linux/bpf_lsm.h > +++ b/include/linux/bpf_lsm.h > @@ -8,27 +8,51 @@ > #define _LINUX_BPF_LSM_H > > #include <linux/bpf.h> > +#include <linux/jump_label.h> > > #ifdef CONFIG_BPF_LSM > > +#define LSM_HOOK(RET, NAME, ...) \ > +DECLARE_STATIC_KEY_FALSE(bpf_lsm_key_##NAME); \ > +void bpf_lsm_##NAME##_set_enabled(bool value); > +#include <linux/lsm_hook_names.h> > +#undef LSM_HOOK This is an amazing amount of macro magic. You're creating dependencies that will make changes to the infrastructure much more difficult. I think. It's really hard to tell. At the very least you should have a description of what this accomplishes, as it's far from obvious. > + > #define LSM_HOOK(RET, NAME, ...) RET bpf_lsm_##NAME(__VA_ARGS__); > #include <linux/lsm_hook_names.h> > #undef LSM_HOOK > > -#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) bpf_lsm_##FUNC(__VA_ARGS__) > +#define HAS_BPF_LSM_PROG(FUNC) (static_branch_unlikely(&bpf_lsm_key_##FUNC)) > + > +#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) \ > + do { \ > + if (HAS_BPF_LSM_PROG(FUNC)) \ > + bpf_lsm_##FUNC(__VA_ARGS__); \ > + } while (0) > + > #define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) ({ \ > do { \ > - if (RC == 0) \ > - RC = bpf_lsm_##FUNC(__VA_ARGS__); \ > + if (HAS_BPF_LSM_PROG(FUNC)) { \ > + if (RC == 0) \ > + RC = bpf_lsm_##FUNC(__VA_ARGS__); \ > + } \ > } while (0); \ > RC; \ > }) > > +int bpf_lsm_set_enabled(const char *name, bool value); > + > #else /* !CONFIG_BPF_LSM */ > > +#define HAS_BPF_LSM_PROG false > #define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) (RC) > #define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) > > +static inline int bpf_lsm_set_enabled(const char *name, bool value) > +{ > + 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 abc847c9b9a1..d7c44433c003 100644 > --- a/kernel/bpf/bpf_lsm.c > +++ b/kernel/bpf/bpf_lsm.c > @@ -8,6 +8,20 @@ > #include <linux/bpf.h> > #include <linux/btf.h> > #include <linux/bpf_lsm.h> > +#include <linux/jump_label.h> > +#include <linux/kallsyms.h> > + > +#define LSM_HOOK(RET, NAME, ...) \ > + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##NAME); \ > + void bpf_lsm_##NAME##_set_enabled(bool value) \ > + { \ > + if (value) \ > + static_branch_enable(&bpf_lsm_key_##NAME); \ > + else \ > + static_branch_disable(&bpf_lsm_key_##NAME); \ > + } > +#include <linux/lsm_hook_names.h> > +#undef LSM_HOOK > > /* For every LSM hook that allows attachment of BPF programs, declare a NOP > * function where a BPF program can be attached as an fexit trampoline. > @@ -24,6 +38,20 @@ > #include <linux/lsm_hook_names.h> > #undef LSM_HOOK > > +int bpf_lsm_set_enabled(const char *name, bool value) > +{ > + char toggle_fn_name[KSYM_NAME_LEN]; > + void (*toggle_fn)(bool value); > + > + snprintf(toggle_fn_name, KSYM_NAME_LEN, "%s_set_enabled", name); > + toggle_fn = (void *)kallsyms_lookup_name(toggle_fn_name); > + if (!toggle_fn) > + return -ESRCH; > + > + toggle_fn(value); > + return 0; > +} > + > const struct bpf_prog_ops lsm_prog_ops = { > }; > > diff --git a/security/security.c b/security/security.c > index aa111392a700..569cc07d5e34 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -804,6 +804,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) > break; > } > } > +#ifdef CONFIG_BPF_LSM > + if (HAS_BPF_LSM_PROG(vm_enough_memory)) { > + rc = bpf_lsm_vm_enough_memory(mm, pages); > + if (rc <= 0) > + cap_sys_admin = 0; > + } > +#endif > return __vm_enough_memory(mm, pages, cap_sys_admin); > } > > @@ -1350,6 +1357,13 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf > if (rc != -EOPNOTSUPP) > return rc; > } > +#ifdef CONFIG_BPF_LSM > + if (HAS_BPF_LSM_PROG(inode_getsecurity)) { > + rc = bpf_lsm_inode_getsecurity(inode, name, buffer, alloc); > + if (rc != -EOPNOTSUPP) > + return rc; > + } > +#endif > return -EOPNOTSUPP; > } > > @@ -1369,6 +1383,14 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void > if (rc != -EOPNOTSUPP) > return rc; > } > +#ifdef CONFIG_BPF_LSM > + if (HAS_BPF_LSM_PROG(inode_setsecurity)) { > + rc = bpf_lsm_inode_setsecurity(inode, name, value, size, > + flags); > + if (rc != -EOPNOTSUPP) > + return rc; > + } > +#endif > return -EOPNOTSUPP; > } > > @@ -1754,6 +1776,12 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, > break; > } > } > +#ifdef CONFIG_BPF_LSM > + if (HAS_BPF_LSM_PROG(task_prctl)) { > + if (rc == -ENOSYS) > + rc = bpf_lsm_task_prctl(option, arg2, arg3, arg4, arg5); > + } > +#endif > return rc; > } > > @@ -2334,6 +2362,10 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x, > rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl); > break; > } > +#ifdef CONFIG_BPF_LSM > + if (HAS_BPF_LSM_PROG(xfrm_state_pol_flow_match)) > + rc = bpf_lsm_xfrm_state_pol_flow_match(x, xp, fl); > +#endif > return rc; > } >
On Fri, 21 Feb 2020, Casey Schaufler wrote: > On 2/20/2020 9:52 AM, KP Singh wrote: > > From: KP Singh <kpsingh@google.com> > > Again, sorry for trimming the CC list, but thunderbird ... Fix your mail client, please.
On Thu, Feb 20, 2020 at 06:52:46PM +0100, KP Singh wrote: > index aa111392a700..569cc07d5e34 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -804,6 +804,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) > break; > } > } > +#ifdef CONFIG_BPF_LSM > + if (HAS_BPF_LSM_PROG(vm_enough_memory)) { > + rc = bpf_lsm_vm_enough_memory(mm, pages); > + if (rc <= 0) > + cap_sys_admin = 0; > + } > +#endif This pattern of using #ifdef in code is not considered best practice. Using in-code IS_ENABLED(CONFIG_BPF_LSM) is preferred. But since this pattern always uses HAS_BPF_LSM_PROG(), you could fold the IS_ENABLED() into the definition of HAS_BPF_LSM_PROG itself -- or more likely, have the macro defined as: #ifdef CONFIG_BPF_LSM # define HAS_BPF_LSM_PROG(x) ....existing implementation.... #else # define HAS_BPF_LSM_PROG(x) false #endif Then none of these ifdefs are needed.
diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h index f867f72f6aa9..53dcda8ace01 100644 --- a/include/linux/bpf_lsm.h +++ b/include/linux/bpf_lsm.h @@ -8,27 +8,51 @@ #define _LINUX_BPF_LSM_H #include <linux/bpf.h> +#include <linux/jump_label.h> #ifdef CONFIG_BPF_LSM +#define LSM_HOOK(RET, NAME, ...) \ +DECLARE_STATIC_KEY_FALSE(bpf_lsm_key_##NAME); \ +void bpf_lsm_##NAME##_set_enabled(bool value); +#include <linux/lsm_hook_names.h> +#undef LSM_HOOK + #define LSM_HOOK(RET, NAME, ...) RET bpf_lsm_##NAME(__VA_ARGS__); #include <linux/lsm_hook_names.h> #undef LSM_HOOK -#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) bpf_lsm_##FUNC(__VA_ARGS__) +#define HAS_BPF_LSM_PROG(FUNC) (static_branch_unlikely(&bpf_lsm_key_##FUNC)) + +#define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) \ + do { \ + if (HAS_BPF_LSM_PROG(FUNC)) \ + bpf_lsm_##FUNC(__VA_ARGS__); \ + } while (0) + #define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) ({ \ do { \ - if (RC == 0) \ - RC = bpf_lsm_##FUNC(__VA_ARGS__); \ + if (HAS_BPF_LSM_PROG(FUNC)) { \ + if (RC == 0) \ + RC = bpf_lsm_##FUNC(__VA_ARGS__); \ + } \ } while (0); \ RC; \ }) +int bpf_lsm_set_enabled(const char *name, bool value); + #else /* !CONFIG_BPF_LSM */ +#define HAS_BPF_LSM_PROG false #define RUN_BPF_LSM_INT_PROGS(RC, FUNC, ...) (RC) #define RUN_BPF_LSM_VOID_PROGS(FUNC, ...) +static inline int bpf_lsm_set_enabled(const char *name, bool value) +{ + 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 abc847c9b9a1..d7c44433c003 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -8,6 +8,20 @@ #include <linux/bpf.h> #include <linux/btf.h> #include <linux/bpf_lsm.h> +#include <linux/jump_label.h> +#include <linux/kallsyms.h> + +#define LSM_HOOK(RET, NAME, ...) \ + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##NAME); \ + void bpf_lsm_##NAME##_set_enabled(bool value) \ + { \ + if (value) \ + static_branch_enable(&bpf_lsm_key_##NAME); \ + else \ + static_branch_disable(&bpf_lsm_key_##NAME); \ + } +#include <linux/lsm_hook_names.h> +#undef LSM_HOOK /* For every LSM hook that allows attachment of BPF programs, declare a NOP * function where a BPF program can be attached as an fexit trampoline. @@ -24,6 +38,20 @@ #include <linux/lsm_hook_names.h> #undef LSM_HOOK +int bpf_lsm_set_enabled(const char *name, bool value) +{ + char toggle_fn_name[KSYM_NAME_LEN]; + void (*toggle_fn)(bool value); + + snprintf(toggle_fn_name, KSYM_NAME_LEN, "%s_set_enabled", name); + toggle_fn = (void *)kallsyms_lookup_name(toggle_fn_name); + if (!toggle_fn) + return -ESRCH; + + toggle_fn(value); + return 0; +} + const struct bpf_prog_ops lsm_prog_ops = { }; diff --git a/security/security.c b/security/security.c index aa111392a700..569cc07d5e34 100644 --- a/security/security.c +++ b/security/security.c @@ -804,6 +804,13 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) break; } } +#ifdef CONFIG_BPF_LSM + if (HAS_BPF_LSM_PROG(vm_enough_memory)) { + rc = bpf_lsm_vm_enough_memory(mm, pages); + if (rc <= 0) + cap_sys_admin = 0; + } +#endif return __vm_enough_memory(mm, pages, cap_sys_admin); } @@ -1350,6 +1357,13 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf if (rc != -EOPNOTSUPP) return rc; } +#ifdef CONFIG_BPF_LSM + if (HAS_BPF_LSM_PROG(inode_getsecurity)) { + rc = bpf_lsm_inode_getsecurity(inode, name, buffer, alloc); + if (rc != -EOPNOTSUPP) + return rc; + } +#endif return -EOPNOTSUPP; } @@ -1369,6 +1383,14 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void if (rc != -EOPNOTSUPP) return rc; } +#ifdef CONFIG_BPF_LSM + if (HAS_BPF_LSM_PROG(inode_setsecurity)) { + rc = bpf_lsm_inode_setsecurity(inode, name, value, size, + flags); + if (rc != -EOPNOTSUPP) + return rc; + } +#endif return -EOPNOTSUPP; } @@ -1754,6 +1776,12 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3, break; } } +#ifdef CONFIG_BPF_LSM + if (HAS_BPF_LSM_PROG(task_prctl)) { + if (rc == -ENOSYS) + rc = bpf_lsm_task_prctl(option, arg2, arg3, arg4, arg5); + } +#endif return rc; } @@ -2334,6 +2362,10 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x, rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl); break; } +#ifdef CONFIG_BPF_LSM + if (HAS_BPF_LSM_PROG(xfrm_state_pol_flow_match)) + rc = bpf_lsm_xfrm_state_pol_flow_match(x, xp, fl); +#endif return rc; }