[PATCH] security: fix no-op hook logic in security_inode_{set, remove}xattr()

[Ondrej Mosnacek]

These two hooks currently work like this:
1. If no LSM registers the hook, cap_inode_{set,remove}xattr() is
   called.
2. If an LSM hook call returns 0, the loop continues to call further
   LSMs (and only stops on an error return value).
3. The "default" return value is 0, so e.g. the default BPF LSM hook
   just returns 0.

This works if BPF LSM is enabled along with SELinux or SMACK (or not
enabled at all), but if it's the only LSM implementing the hook, then
the cap_inode_{set,remove}xattr() is erroneously skipped.

Fix this by using 1 as the default return value and make the loop
recognize it as a no-op return value (i.e. if an LSM returns this value
it is treated as if it wasn't called at all). The final logic is similar
to that of security_fs_context_parse_param().

Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
---
 include/linux/lsm_hook_defs.h |  4 ++--
 security/security.c           | 45 +++++++++++++++++++++++++----------
 2 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 76458b6d53da..a281db62b665 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -137,14 +137,14 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode,
 LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask)
 LSM_HOOK(int, 0, inode_setattr, struct dentry *dentry, struct iattr *attr)
 LSM_HOOK(int, 0, inode_getattr, const struct path *path)
-LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap,
+LSM_HOOK(int, 1, inode_setxattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *name, const void *value,
 	 size_t size, int flags)
 LSM_HOOK(void, LSM_RET_VOID, inode_post_setxattr, struct dentry *dentry,
 	 const char *name, const void *value, size_t size, int flags)
 LSM_HOOK(int, 0, inode_getxattr, struct dentry *dentry, const char *name)
 LSM_HOOK(int, 0, inode_listxattr, struct dentry *dentry)
-LSM_HOOK(int, 0, inode_removexattr, struct mnt_idmap *idmap,
+LSM_HOOK(int, 1, inode_removexattr, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *name)
 LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *acl_name, struct posix_acl *kacl)
diff --git a/security/security.c b/security/security.c
index 3aaad75c9ce8..cedd6c150bdd 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2254,7 +2254,8 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
 			    struct dentry *dentry, const char *name,
 			    const void *value, size_t size, int flags)
 {
-	int ret;
+	struct security_hook_list *hp;
+	int ret = LSM_RET_DEFAULT(inode_setxattr);
 
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
@@ -2262,13 +2263,22 @@ int security_inode_setxattr(struct mnt_idmap *idmap,
 	 * SELinux and Smack integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
-	ret = call_int_hook(inode_setxattr, 1, idmap, dentry, name, value,
-			    size, flags);
-
-	if (ret == 1)
+	hlist_for_each_entry(hp, &security_hook_heads.inode_setxattr,
+			     list) {
+		int trc = hp->hook.inode_setxattr(idmap, dentry, name,
+						  value, size, flags);
+		if (trc == LSM_RET_DEFAULT(inode_setxattr))
+			continue;
+		if (trc)
+			return trc;
+		ret = 0;
+	}
+	/* rc can only be either LSM_RET_DEFAULT(...) or 0 here */
+	if (ret == LSM_RET_DEFAULT(inode_setxattr)) {
 		ret = cap_inode_setxattr(dentry, name, value, size, flags);
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
+	}
 	ret = ima_inode_setxattr(dentry, name, value, size);
 	if (ret)
 		return ret;
@@ -2417,7 +2427,8 @@ int security_inode_listxattr(struct dentry *dentry)
 int security_inode_removexattr(struct mnt_idmap *idmap,
 			       struct dentry *dentry, const char *name)
 {
-	int ret;
+	struct security_hook_list *hp;
+	int ret = LSM_RET_DEFAULT(inode_removexattr);
 
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return 0;
@@ -2425,11 +2436,21 @@ int security_inode_removexattr(struct mnt_idmap *idmap,
 	 * SELinux and Smack integrate the cap call,
 	 * so assume that all LSMs supplying this call do so.
 	 */
-	ret = call_int_hook(inode_removexattr, 1, idmap, dentry, name);
-	if (ret == 1)
+	hlist_for_each_entry(hp, &security_hook_heads.inode_removexattr,
+			     list) {
+		int trc = hp->hook.inode_removexattr(idmap, dentry, name);
+		if (trc == LSM_RET_DEFAULT(inode_removexattr))
+			continue;
+		if (trc)
+			return trc;
+		ret = 0;
+	}
+	/* rc can only be either LSM_RET_DEFAULT(...) or 0 here */
+	if (ret == LSM_RET_DEFAULT(inode_removexattr)) {
 		ret = cap_inode_removexattr(idmap, dentry, name);
-	if (ret)
-		return ret;
+		if (ret)
+			return ret;
+	}
 	ret = ima_inode_removexattr(dentry, name);
 	if (ret)
 		return ret;
-- 
2.43.0