[PATCH linux-next] security: Fix side effects of default BPF LSM hooks

[KP Singh <kpsingh@kernel.org>]

BPF LSM currently has a default implementation for each LSM hooks which
return a default value defined in include/linux/lsm_hook_defs.h. These
hooks should have no functional effect when there is no BPF program
loaded to implement the hook logic.

Some LSM hooks treat any return value of the hook as policy decision
which results in destructive side effects.

This issue and the effects were reported to me by Jann Horn:

For a system configured with CONFIG_BPF_LSM and the bpf lsm is enabled
(via lsm= or CONFIG_LSM) an unprivileged user can vandalize the system
by removing the security.capability xattrs from binaries, preventing
them from working normally:

$ getfattr -d -m- /bin/ping
getfattr: Removing leading '/' from absolute path names
security.capability=0sAQAAAgAgAAAAAAAAAAAAAAAAAAA=

$ setfattr -x security.capability /bin/ping
$ getfattr -d -m- /bin/ping
$ ping 1.2.3.4
$ ping google.com
$ echo $?
2

The above reproduces with:

cat /sys/kernel/security/lsm
capability,apparmor,bpf

But not with SELinux as SELinux does the required check in its LSM hook:

cat /sys/kernel/security/lsm
capability,selinux,bpf

In this case security_inode_removexattr() calls
call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name), which
expects a return value of 1 to mean "no LSM hooks hit" and 0 is
supposed to mean "the LSM decided to permit the access and checked
cap_inode_removexattr"

There are other security hooks that are similarly effected.

In order to reliably fix this issue and also allow LSM Hooks and BPF
programs which implement hook logic to choose to not make a decision
in certain conditions (e.g. when BPF programs are used for auditing),
introduce a special return value LSM_HOOK_NO_EFFECT which can be used
by the hook to indicate to the framework that it does not intend to
make a decision.

Fixes: 520b7aa00d8c ("bpf: lsm: Initialize the BPF LSM hooks")
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: KP Singh <kpsingh@kernel.org>
---
 include/linux/lsm_hooks.h |  6 +++
 kernel/bpf/bpf_lsm.c      | 14 +++++--
 security/security.c       | 78 +++++++++++++++++++++++++++++----------
 3 files changed, 75 insertions(+), 23 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 91c8146649f5..fcf3c2c57127 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1576,6 +1576,12 @@
  *      thread (IORING_SETUP_SQPOLL).
  *
  */
+
+/*
+ * If an LSM hook choses to make no decision. i.e. it's only for auditing or
+ * a default hook like the BPF LSM hooks, it can return LSM_HOOK_NO_EFFECT.
+ */
+ #define LSM_HOOK_NO_EFFECT -INT_MAX
 union security_list_options {
 	#define LSM_HOOK(RET, DEFAULT, NAME, ...) RET (*NAME)(__VA_ARGS__);
 	#include "lsm_hook_defs.h"
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index c1351df9f7ee..52f2fc493c57 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -20,12 +20,18 @@
 /* For every LSM hook that allows attachment of BPF programs, declare a nop
  * function where a BPF program can be attached.
  */
-#define LSM_HOOK(RET, DEFAULT, NAME, ...)	\
-noinline RET bpf_lsm_##NAME(__VA_ARGS__)	\
-{						\
-	return DEFAULT;				\
+#define DEFINE_LSM_HOOK_void(NAME, ...) \
+noinline void bpf_lsm_##NAME(__VA_ARGS__) {}
+
+#define DEFINE_LSM_HOOK_int(NAME, ...)	   \
+noinline int bpf_lsm_##NAME(__VA_ARGS__)   \
+{					   \
+	return LSM_HOOK_NO_EFFECT;	   \
 }
 
+#define LSM_HOOK(RET, DEFAULT, NAME, ...) \
+	DEFINE_LSM_HOOK_##RET(NAME, __VA_ARGS__)
+
 #include <linux/lsm_hook_defs.h>
 #undef LSM_HOOK
 
diff --git a/security/security.c b/security/security.c
index 188b8f782220..3c1b0b00c4a7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -734,11 +734,16 @@ static int lsm_superblock_alloc(struct super_block *sb)
 
 #define call_int_hook(FUNC, IRC, ...) ({			\
 	int RC = IRC;						\
+	int THISRC;						\
+								\
 	do {							\
 		struct security_hook_list *P;			\
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
-			RC = P->hook.FUNC(__VA_ARGS__);		\
+			THISRC = P->hook.FUNC(__VA_ARGS__);	\
+			if (THISRC == LSM_HOOK_NO_EFFECT)	\
+				continue;			\
+			RC = THISRC;				\
 			if (RC != 0)				\
 				break;				\
 		}						\
@@ -831,7 +836,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 {
 	struct security_hook_list *hp;
 	int cap_sys_admin = 1;
-	int rc;
+	int rc, thisrc;
 
 	/*
 	 * The module will respond with a positive value if
@@ -841,7 +846,11 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	 * thinks it should not be set it won't.
 	 */
 	hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
-		rc = hp->hook.vm_enough_memory(mm, pages);
+		thisrc = hp->hook.vm_enough_memory(mm, pages);
+		if (thisrc == LSM_HOOK_NO_EFFECT)
+			continue;
+		rc = thisrc;
+
 		if (rc <= 0) {
 			cap_sys_admin = 0;
 			break;
@@ -895,6 +904,8 @@ int security_fs_context_parse_param(struct fs_context *fc,
 	hlist_for_each_entry(hp, &security_hook_heads.fs_context_parse_param,
 			     list) {
 		trc = hp->hook.fs_context_parse_param(fc, param);
+		if (trc == LSM_HOOK_NO_EFFECT)
+			continue;
 		if (trc == 0)
 			rc = 0;
 		else if (trc != -ENOPARAM)
@@ -1063,14 +1074,17 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
 				  u32 *ctxlen)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc, thisrc;
 
 	/*
 	 * Only one module will provide a security context.
 	 */
 	hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) {
-		rc = hp->hook.dentry_init_security(dentry, mode, name,
-						   xattr_name, ctx, ctxlen);
+		thisrc = hp->hook.dentry_init_security(dentry, mode, name,
+						       xattr_name, ctx, ctxlen);
+		if (thisrc == LSM_HOOK_NO_EFFECT)
+			continue;
+		rc = thisrc;
 		if (rc != LSM_RET_DEFAULT(dentry_init_security))
 			return rc;
 	}
@@ -1430,7 +1444,7 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
 			       void **buffer, bool alloc)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc, thisrc;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return LSM_RET_DEFAULT(inode_getsecurity);
@@ -1438,7 +1452,10 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
 	 * Only one module will provide an attribute with a given name.
 	 */
 	hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
-		rc = hp->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
+		thisrc = hp->hook.inode_getsecurity(mnt_userns, inode, name, buffer, alloc);
+		if (thisrc == LSM_HOOK_NO_EFFECT)
+			continue;
+		rc = thisrc;
 		if (rc != LSM_RET_DEFAULT(inode_getsecurity))
 			return rc;
 	}
@@ -1448,7 +1465,7 @@ int security_inode_getsecurity(struct user_namespace *mnt_userns,
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc, thisrc;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return LSM_RET_DEFAULT(inode_setsecurity);
@@ -1456,8 +1473,11 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
 	 * Only one module will provide an attribute with a given name.
 	 */
 	hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
-		rc = hp->hook.inode_setsecurity(inode, name, value, size,
-								flags);
+		thisrc = hp->hook.inode_setsecurity(inode, name, value, size,
+						    flags);
+		if (thisrc == LSM_HOOK_NO_EFFECT)
+			continue;
+		rc = thisrc;
 		if (rc != LSM_RET_DEFAULT(inode_setsecurity))
 			return rc;
 	}
@@ -1486,7 +1506,7 @@ EXPORT_SYMBOL(security_inode_copy_up);
 int security_inode_copy_up_xattr(const char *name)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc, thisrc;
 
 	/*
 	 * The implementation can return 0 (accept the xattr), 1 (discard the
@@ -1495,7 +1515,10 @@ int security_inode_copy_up_xattr(const char *name)
 	 */
 	hlist_for_each_entry(hp,
 		&security_hook_heads.inode_copy_up_xattr, list) {
-		rc = hp->hook.inode_copy_up_xattr(name);
+		thisrc = hp->hook.inode_copy_up_xattr(name);
+		if (thisrc == LSM_HOOK_NO_EFFECT)
+			continue;
+		rc = thisrc;
 		if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
 			return rc;
 	}
@@ -1889,6 +1912,8 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 
 	hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
 		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
+		if (thisrc == LSM_HOOK_NO_EFFECT)
+			continue;
 		if (thisrc != LSM_RET_DEFAULT(task_prctl)) {
 			rc = thisrc;
 			if (thisrc != 0)
@@ -2055,11 +2080,16 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
 				char **value)
 {
 	struct security_hook_list *hp;
+	int rc;
 
 	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
 		if (lsm != NULL && strcmp(lsm, hp->lsm))
 			continue;
-		return hp->hook.getprocattr(p, name, value);
+		rc = hp->hook.getprocattr(p, name, value);
+		if (rc == LSM_HOOK_NO_EFFECT)
+			continue;
+		else
+			return rc;
 	}
 	return LSM_RET_DEFAULT(getprocattr);
 }
@@ -2068,11 +2098,16 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
 			 size_t size)
 {
 	struct security_hook_list *hp;
+	int rc;
 
 	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
 		if (lsm != NULL && strcmp(lsm, hp->lsm))
 			continue;
-		return hp->hook.setprocattr(name, value, size);
+		rc = hp->hook.setprocattr(name, value, size);
+		if (rc == LSM_HOOK_NO_EFFECT)
+			continue;
+		else
+			return rc;
 	}
 	return LSM_RET_DEFAULT(setprocattr);
 }
@@ -2091,14 +2126,17 @@ EXPORT_SYMBOL(security_ismaclabel);
 int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc, thisrc;
 
 	/*
 	 * Currently, only one LSM can implement secid_to_secctx (i.e this
 	 * LSM hook is not "stackable").
 	 */
 	hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx, list) {
-		rc = hp->hook.secid_to_secctx(secid, secdata, seclen);
+		thisrc = hp->hook.secid_to_secctx(secid, secdata, seclen);
+		if (thisrc == LSM_HOOK_NO_EFFECT)
+			continue;
+		rc = thisrc;
 		if (rc != LSM_RET_DEFAULT(secid_to_secctx))
 			return rc;
 	}
@@ -2509,9 +2547,11 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
 	hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
 				list) {
 		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, flic);
-		break;
+		if (rc == LSM_HOOK_NO_EFFECT)
+			continue;
+		return rc;
 	}
-	return rc;
+	return LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
 }
 
 int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
-- 
2.36.1.476.g0c4daa206d-goog