| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <bb32f155-5213-71df-c679-85c614c0ac26@schaufler-ca.com>
Date: Fri, 21 Feb 2020 10:57:39 -0800
From: Casey Schaufler <casey@...aufler-ca.com>
To: KP Singh <kpsingh@...omium.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
Linux Security Module list
<linux-security-module@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH bpf-next v4 4/8] bpf: lsm: Add support for
enabling/disabling BPF hooks
On 2/20/2020 9:52 AM, KP Singh wrote:
> From: KP Singh <kpsingh@...gle.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@...gle.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;
> }
>
Powered by blists - more mailing lists