[bpf-next,v4,4/8] bpf: lsm: Add support for enabling/disabling BPF hooks
diff mbox series

Message ID 20200220175250.10795-5-kpsingh@chromium.org
State New
Headers show
Series
  • MAC and Audit policy using eBPF (KRSI)
Related show

Commit Message

KP Singh Feb. 20, 2020, 5:52 p.m. UTC
From: KP Singh <kpsingh@google.com>

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(-)

Comments

Casey Schaufler Feb. 21, 2020, 6:57 p.m. UTC | #1
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;
>  }
>
James Morris Feb. 21, 2020, 7:11 p.m. UTC | #2
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.
Kees Cook Feb. 22, 2020, 4:26 a.m. UTC | #3
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.

Patch
diff mbox series

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;
 }