On 6/9/2022 4:46 PM, KP Singh wrote:
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.
This shouldn't come as a surprise.
https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg2281257.html
is just one place where this sort of issue has been discussed.
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.
The LSM infrastructure already has a convention of returning
-EOPNOTSUPP for this condition. Why add another value to check?'
If you really want LSM_HOOK_NO_EFFECT you need to convert all the
hooks, in both the infrastructure and the modules, that use
-EOPNOTSUPP as well as what you have below.
Fixes: 520b7aa00d8c ("bpf: lsm: Initialize the BPF LSM hooks")
Reported-by: Jann Horn <jannh@xxxxxxxxxx>
Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
---
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
How are you going to ensure this will never, ever conflict with
a legitimate error code? At the very least this needs to be in errno.h,
not here.
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);
This could be a case where BPF is already broken.
To match the documented interface the module should return
1 if it doesn't care.
+ 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;
OK, so the LSM convention has to take a back seat to the mount
convention in this case. The BPF hook should adhere to that convention
and return -ENOPARAM when it doesn't care.
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;
The BPF module should return LSM_RET_DEFAULT(dentry_init_security),
which I believe is -EOPNOTSUPP. BTW, if BPF ever supplies a hook for
this that does something I expect there will be tears.
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;
The BPF module should return LSM_RET_DEFAULT(inode_getsecurity).
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;
Again.
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;
Again.
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;
This shouldn't be necessary as the LSM is explicitly called out.
}
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;
Same here.
}
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;
As with dentry_init, you're playing with fire if BPF expects to use this hook.
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;
This is an odd duck hook, and SELinux will be broken if BPF supplied a real hook.
}
- return rc;
+ return LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
}
int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
