[PATCH bpf-next v4 00/20] Add return value range check for BPF LSM

[PATCH bpf-next v4 00/20] Add return value range check for BPF LSM

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

LSM BPF prog returning a positive number attached to the hook
file_alloc_security makes kernel panic. 

Here is a panic log:

[  441.235774] BUG: kernel NULL pointer dereference, address: 00000000000009
[  441.236748] #PF: supervisor write access in kernel mode
[  441.237429] #PF: error_code(0x0002) - not-present page
[  441.238119] PGD 800000000b02f067 P4D 800000000b02f067 PUD b031067 PMD 0
[  441.238990] Oops: 0002 [#1] PREEMPT SMP PTI
[  441.239546] CPU: 0 PID: 347 Comm: loader Not tainted 6.8.0-rc6-gafe0cbf23373 #22
[  441.240496] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b4
[  441.241933] RIP: 0010:alloc_file+0x4b/0x190
[  441.242485] Code: 8b 04 25 c0 3c 1f 00 48 8b b0 30 0c 00 00 e8 9c fe ff ff 48 3d 00 f0 ff fb
[  441.244820] RSP: 0018:ffffc90000c67c40 EFLAGS: 00010203
[  441.245484] RAX: ffff888006a891a0 RBX: ffffffff8223bd00 RCX: 0000000035b08000
[  441.246391] RDX: ffff88800b95f7b0 RSI: 00000000001fc110 RDI: f089cd0b8088ffff
[  441.247294] RBP: ffffc90000c67c58 R08: 0000000000000001 R09: 0000000000000001
[  441.248209] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
[  441.249108] R13: ffffc90000c67c78 R14: ffffffff8223bd00 R15: fffffffffffffff4
[  441.250007] FS:  00000000005f3300(0000) GS:ffff88803ec00000(0000) knlGS:0000000000000000
[  441.251053] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  441.251788] CR2: 00000000000001a9 CR3: 000000000bdc4003 CR4: 0000000000170ef0
[  441.252688] Call Trace:
[  441.253011]  <TASK>
[  441.253296]  ? __die+0x24/0x70
[  441.253702]  ? page_fault_oops+0x15b/0x480
[  441.254236]  ? fixup_exception+0x26/0x330
[  441.254750]  ? exc_page_fault+0x6d/0x1c0
[  441.255257]  ? asm_exc_page_fault+0x26/0x30
[  441.255792]  ? alloc_file+0x4b/0x190
[  441.256257]  alloc_file_pseudo+0x9f/0xf0
[  441.256760]  __anon_inode_getfile+0x87/0x190
[  441.257311]  ? lock_release+0x14e/0x3f0
[  441.257808]  bpf_link_prime+0xe8/0x1d0
[  441.258315]  bpf_tracing_prog_attach+0x311/0x570
[  441.258916]  ? __pfx_bpf_lsm_file_alloc_security+0x10/0x10
[  441.259605]  __sys_bpf+0x1bb7/0x2dc0
[  441.260070]  __x64_sys_bpf+0x20/0x30
[  441.260533]  do_syscall_64+0x72/0x140
[  441.261004]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
[  441.261643] RIP: 0033:0x4b0349
[  441.262045] Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 88
[  441.264355] RSP: 002b:00007fff74daee38 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
[  441.265293] RAX: ffffffffffffffda RBX: 00007fff74daef30 RCX: 00000000004b0349
[  441.266187] RDX: 0000000000000040 RSI: 00007fff74daee50 RDI: 000000000000001c
[  441.267114] RBP: 000000000000001b R08: 00000000005ef820 R09: 0000000000000000
[  441.268018] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
[  441.268907] R13: 0000000000000004 R14: 00000000005ef018 R15: 00000000004004e8

This is because the filesystem uses IS_ERR to check if the return value
is an error code. If it is not, the filesystem takes the return value
as a file pointer. Since the positive number returned by the BPF prog
is not a real file pointer, this misinterpretation causes a panic.

Since other LSM modules always return either a negative error code
or a valid pointer, this specific issue only exists in BPF LSM. The
proposed solution is to reject LSM BPF progs returning unexpected
values in the verifier. This patch set adds return value check to
ensure only BPF progs returning expected values are accepted.

Since each LSM hook has different excepted return values, we need to
know the expected return values for each individual hook to do the
check. Earlier versions of the patch set used LSM hook annotations
to specify the return value range for each hook. Based on Paul's
suggestion, current version gets rid of such annotations and instead
converts hook return values to a common pattern: return 0 on success
and negative error code on failure.

Basically, LSM hooks are divided into two types: hooks that return a
negative error code and zero or other values, and hooks that do not
return a negative error code. This patch set converts all hooks of the
first type and part of the second type to return 0 on success and a
negative error code on failure (see patches 1-10). For certain hooks,
like ismaclabel and inode_xattr_skipcap, the hook name already imply
that returning 0 or 1 is the best choice, so they are not converted.
There are four unconverted hooks. Except for ismaclabel, which is not
used by BPF LSM, the other three are specified with a BTF ID list to
only return 0 or 1.

v4:
1. remove LSM_HOOK return value annotaion and convert LSM hook return
   value to a common patern: return 0 on success and negative error code
   on failure (patch 1-10)
2. enable BPF LSM progs to read and write output params (patch 12)
3. add a special case for bitwise AND on range [-1, 0] (patch 16)
4. add a 32-bit comparing flag for retval_range_within (patch 15)
5. collect ACKs, style fix, etc

v3: https://lore.kernel.org/bpf/20240411122752.2873562-1-xukuohai@huaweicloud.com/
1. Fix incorrect lsm hook return value ranges, and add disabled hook
   list for bpf lsm, and merge two LSM_RET_INT patches. (KP Singh)
2. Avoid bpf lsm progs attached to different hooks to call each other
   with tail call
3. Fix a CI failure caused by false rejection of AND operation
4. Add tests

v2: https://lore.kernel.org/bpf/20240325095653.1720123-1-xukuohai@huaweicloud.com/
fix bpf ci failure

v1: https://lore.kernel.org/bpf/20240316122359.1073787-1-xukuohai@huaweicloud.com/

Xu Kuohai (20):
  lsm: Refactor return value of LSM hook vm_enough_memory
  lsm: Refactor return value of LSM hook inode_need_killpriv
  lsm: Refactor return value of LSM hook inode_getsecurity
  lsm: Refactor return value of LSM hook inode_listsecurity
  lsm: Refactor return value of LSM hook inode_copy_up_xattr
  lsm: Refactor return value of LSM hook getselfattr
  lsm: Refactor return value of LSM hook setprocattr
  lsm: Refactor return value of LSM hook getprocattr
  lsm: Refactor return value of LSM hook key_getsecurity
  lsm: Refactor return value of LSM hook audit_rule_match
  bpf, lsm: Add disabled BPF LSM hook list
  bpf, lsm: Enable BPF LSM prog to read/write return value parameters
  bpf, lsm: Add check for BPF LSM return value
  bpf: Prevent tail call between progs attached to different hooks
  bpf: Fix compare error in function retval_range_within
  bpf: Add a special case for bitwise AND on range [-1, 0]
  selftests/bpf: Avoid load failure for token_lsm.c
  selftests/bpf: Add return value checks for failed tests
  selftests/bpf: Add test for lsm tail call
  selftests/bpf: Add verifier tests for bpf lsm

 fs/attr.c                                     |   5 +-
 fs/inode.c                                    |   4 +-
 fs/nfs/nfs4proc.c                             |   5 +-
 fs/overlayfs/copy_up.c                        |   6 +-
 fs/proc/base.c                                |  10 +-
 fs/xattr.c                                    |  24 +-
 include/linux/bpf.h                           |   2 +
 include/linux/bpf_lsm.h                       |  15 +
 include/linux/lsm_hook_defs.h                 |  22 +-
 include/linux/security.h                      |  62 ++--
 include/linux/tnum.h                          |   3 +
 kernel/bpf/bpf_lsm.c                          |  64 +++-
 kernel/bpf/btf.c                              |  21 +-
 kernel/bpf/core.c                             |  21 +-
 kernel/bpf/tnum.c                             |  25 ++
 kernel/bpf/verifier.c                         | 173 ++++++++++-
 net/socket.c                                  |   9 +-
 security/apparmor/audit.c                     |  22 +-
 security/apparmor/include/audit.h             |   2 +-
 security/apparmor/lsm.c                       |  22 +-
 security/commoncap.c                          |  32 +-
 security/integrity/evm/evm_main.c             |   2 +-
 security/keys/keyctl.c                        |  11 +-
 security/lsm_syscalls.c                       |   6 +-
 security/security.c                           | 167 ++++++++---
 security/selinux/hooks.c                      |  94 +++---
 security/selinux/include/audit.h              |   8 +-
 security/selinux/ss/services.c                |  54 ++--
 security/smack/smack_lsm.c                    | 104 ++++---
 .../selftests/bpf/prog_tests/test_lsm.c       |  46 ++-
 .../selftests/bpf/prog_tests/verifier.c       |   2 +
 tools/testing/selftests/bpf/progs/err.h       |  10 +
 .../selftests/bpf/progs/lsm_tailcall.c        |  34 +++
 .../selftests/bpf/progs/test_sig_in_xattr.c   |   4 +
 .../bpf/progs/test_verify_pkcs7_sig.c         |   8 +-
 tools/testing/selftests/bpf/progs/token_lsm.c |   4 +-
 .../bpf/progs/verifier_global_subprogs.c      |   7 +-
 .../selftests/bpf/progs/verifier_lsm.c        | 274 ++++++++++++++++++
 38 files changed, 1098 insertions(+), 286 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/lsm_tailcall.c
 create mode 100644 tools/testing/selftests/bpf/progs/verifier_lsm.c

-- 
2.30.2

[PATCH bpf-next v4 01/20] lsm: Refactor return value of LSM hook vm_enough_memory

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook vm_enough_memory to 0 or a negative error code.

Before:
- Hook vm_enough_memory returns 1 if permission is granted, 0 if not.
- LSM_RET_DEFAULT(vm_enough_memory_mm) is 1.

After:
- Hook vm_enough_memory reutrns 0 if permission is granted, negative
  error code if not.
- LSM_RET_DEFAULT(vm_enough_memory_mm) is 0.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  2 +-
 security/commoncap.c          | 11 +++--------
 security/security.c           | 11 +++++------
 security/selinux/hooks.c      | 15 ++++-----------
 5 files changed, 14 insertions(+), 27 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 44488b1ab9a9..e6e6f8473955 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -48,7 +48,7 @@ LSM_HOOK(int, 0, quota_on, struct dentry *dentry)
 LSM_HOOK(int, 0, syslog, int type)
 LSM_HOOK(int, 0, settime, const struct timespec64 *ts,
 	 const struct timezone *tz)
-LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages)
+LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages)
 LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm)
 LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct file *file)
 LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
diff --git a/include/linux/security.h b/include/linux/security.h
index de3af33e6ff5..454f96307cb9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -634,7 +634,7 @@ static inline int security_settime64(const struct timespec64 *ts,
 
 static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 {
-	return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages));
+	return __vm_enough_memory(mm, pages, !cap_vm_enough_memory(mm, pages));
 }
 
 static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm)
diff --git a/security/commoncap.c b/security/commoncap.c
index 162d96b3a676..cefad323a0b1 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1396,17 +1396,12 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
  * Determine whether the allocation of a new virtual mapping by the current
  * task is permitted.
  *
- * Return: 1 if permission is granted, 0 if not.
+ * Return: 0 if permission granted, negative error code if not.
  */
 int cap_vm_enough_memory(struct mm_struct *mm, long pages)
 {
-	int cap_sys_admin = 0;
-
-	if (cap_capable(current_cred(), &init_user_ns,
-				CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) == 0)
-		cap_sys_admin = 1;
-
-	return cap_sys_admin;
+	return cap_capable(current_cred(), &init_user_ns, CAP_SYS_ADMIN,
+			   CAP_OPT_NOAUDIT);
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index e5ca08789f74..3475f0cab3da 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1115,15 +1115,14 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	int rc;
 
 	/*
-	 * The module will respond with a positive value if
-	 * it thinks the __vm_enough_memory() call should be
-	 * made with the cap_sys_admin set. If all of the modules
-	 * agree that it should be set it will. If any module
-	 * thinks it should not be set it won't.
+	 * The module will respond with 0 if it thinks the __vm_enough_memory()
+	 * call should be made with the cap_sys_admin set. If all of the modules
+	 * agree that it should be set it will. If any module 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);
-		if (rc <= 0) {
+		if (rc < 0) {
 			cap_sys_admin = 0;
 			break;
 		}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7eed331e90f0..9cd5a8f1f6a3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2202,23 +2202,16 @@ static int selinux_syslog(int type)
 }
 
 /*
- * Check that a process has enough memory to allocate a new virtual
- * mapping. 0 means there is enough memory for the allocation to
- * succeed and -ENOMEM implies there is not.
+ * Check permission for allocating a new virtual mapping. Returns
+ * 0 if permission is granted, negative error code if not.
  *
  * Do not audit the selinux permission check, as this is applied to all
  * processes that allocate mappings.
  */
 static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
 {
-	int rc, cap_sys_admin = 0;
-
-	rc = cred_has_capability(current_cred(), CAP_SYS_ADMIN,
-				 CAP_OPT_NOAUDIT, true);
-	if (rc == 0)
-		cap_sys_admin = 1;
-
-	return cap_sys_admin;
+	return cred_has_capability(current_cred(), CAP_SYS_ADMIN,
+				   CAP_OPT_NOAUDIT, true);
 }
 
 /* binprm security operations */
-- 
2.30.2

[PATCH bpf-next v4 02/20] lsm: Refactor return value of LSM hook inode_need_killpriv

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook inode_need_killpriv to 0 or a negative error code.

Before:
- Both hook inode_need_killpriv and func security_inode_need_killpriv
  return > 0 if security_inode_killpriv is required, 0 if not, and < 0
  to abort the operation.

After:
- Both hook inode_need_killpriv and func security_inode_need_killpriv
  return 0 on success and a negative error code on failure.
  On success, hook inode_need_killpriv sets output param @need to true
  if security_inode_killpriv is required, and false if not. When @need
  is true, func security_inode_need_killpriv sets ATTR_KILL_PRIV flag
  in @attr; when false, it clears the flag.
  On failure, @need and @attr remains unchanged.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 fs/attr.c                     |  5 ++---
 fs/inode.c                    |  4 +---
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      | 20 ++++++++++++++++----
 security/commoncap.c          | 12 ++++++++----
 security/security.c           | 29 ++++++++++++++++++++++++-----
 6 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/fs/attr.c b/fs/attr.c
index 960a310581eb..aaadc721c982 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -427,11 +427,10 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
 		attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
 
 	if (ia_valid & ATTR_KILL_PRIV) {
-		error = security_inode_need_killpriv(dentry);
+		error = security_inode_need_killpriv(dentry, &ia_valid);
 		if (error < 0)
 			return error;
-		if (error == 0)
-			ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
+		attr->ia_valid = ia_valid;
 	}
 
 	/*
diff --git a/fs/inode.c b/fs/inode.c
index 3a41f83a4ba5..cd335dc3a3bc 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2012,11 +2012,9 @@ int dentry_needs_remove_privs(struct mnt_idmap *idmap,
 		return 0;
 
 	mask = setattr_should_drop_suidgid(idmap, inode);
-	ret = security_inode_need_killpriv(dentry);
+	ret = security_inode_need_killpriv(dentry, &mask);
 	if (ret < 0)
 		return ret;
-	if (ret)
-		mask |= ATTR_KILL_PRIV;
 	return mask;
 }
 
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index e6e6f8473955..964849de424b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -165,7 +165,7 @@ LSM_HOOK(int, 0, inode_remove_acl, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *acl_name)
 LSM_HOOK(void, LSM_RET_VOID, inode_post_remove_acl, struct mnt_idmap *idmap,
 	 struct dentry *dentry, const char *acl_name)
-LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry)
+LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry, bool *need)
 LSM_HOOK(int, 0, inode_killpriv, struct mnt_idmap *idmap,
 	 struct dentry *dentry)
 LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap,
diff --git a/include/linux/security.h b/include/linux/security.h
index 454f96307cb9..1614ef5b2dd2 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -161,7 +161,7 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
 		       const void *value, size_t size, int flags);
 int cap_inode_removexattr(struct mnt_idmap *idmap,
 			  struct dentry *dentry, const char *name);
-int cap_inode_need_killpriv(struct dentry *dentry);
+int cap_inode_need_killpriv(struct dentry *dentry, bool *need);
 int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
 int cap_inode_getsecurity(struct mnt_idmap *idmap,
 			  struct inode *inode, const char *name, void **buffer,
@@ -389,7 +389,7 @@ int security_inode_listxattr(struct dentry *dentry);
 int security_inode_removexattr(struct mnt_idmap *idmap,
 			       struct dentry *dentry, const char *name);
 void security_inode_post_removexattr(struct dentry *dentry, const char *name);
-int security_inode_need_killpriv(struct dentry *dentry);
+int security_inode_need_killpriv(struct dentry *dentry, int *attr);
 int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
 int security_inode_getsecurity(struct mnt_idmap *idmap,
 			       struct inode *inode, const char *name,
@@ -971,9 +971,21 @@ static inline void security_inode_post_removexattr(struct dentry *dentry,
 						   const char *name)
 { }
 
-static inline int security_inode_need_killpriv(struct dentry *dentry)
+static inline int security_inode_need_killpriv(struct dentry *dentry, int *attr)
 {
-	return cap_inode_need_killpriv(dentry);
+	int rc;
+	bool need = false;
+
+	rc = cap_inode_need_killpriv(dentry, &need);
+	if (rc < 0)
+		return rc;
+
+	if (need)
+		*attr |= ATTR_KILL_PRIV;
+	else
+		*attr &= ~ATTR_KILL_PRIV;
+
+	return 0;
 }
 
 static inline int security_inode_killpriv(struct mnt_idmap *idmap,
diff --git a/security/commoncap.c b/security/commoncap.c
index cefad323a0b1..17d6188d22cf 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -286,21 +286,25 @@ int cap_capset(struct cred *new,
 /**
  * cap_inode_need_killpriv - Determine if inode change affects privileges
  * @dentry: The inode/dentry in being changed with change marked ATTR_KILL_PRIV
+ * @need: If inode_killpriv() is needed
  *
  * Determine if an inode having a change applied that's marked ATTR_KILL_PRIV
  * affects the security markings on that inode, and if it is, should
  * inode_killpriv() be invoked or the change rejected.
  *
- * Return: 1 if security.capability has a value, meaning inode_killpriv()
- * is required, 0 otherwise, meaning inode_killpriv() is not required.
+ * Return: Always returns 0. If security.capability has a value, meaning
+ * inode_killpriv() is required, @need is set to true.
  */
-int cap_inode_need_killpriv(struct dentry *dentry)
+int cap_inode_need_killpriv(struct dentry *dentry, bool *need)
 {
 	struct inode *inode = d_backing_inode(dentry);
 	int error;
 
 	error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0);
-	return error > 0;
+	if (error > 0)
+		*need = true;
+
+	return 0;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index 3475f0cab3da..a4abcd86eb36 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2490,17 +2490,36 @@ void security_inode_post_removexattr(struct dentry *dentry, const char *name)
 /**
  * security_inode_need_killpriv() - Check if security_inode_killpriv() required
  * @dentry: associated dentry
+ * @attr: attribute flags
  *
  * Called when an inode has been changed to determine if
  * security_inode_killpriv() should be called.
  *
- * Return: Return <0 on error to abort the inode change operation, return 0 if
- *         security_inode_killpriv() does not need to be called, return >0 if
- *         security_inode_killpriv() does need to be called.
+ * Return: Return 0 on success, negative error code on failure.
+ *         On success, set ATTR_KILL_PRIV flag in @attr when @need is true,
+ *         clears it when false.
  */
-int security_inode_need_killpriv(struct dentry *dentry)
+int security_inode_need_killpriv(struct dentry *dentry, int *attr)
 {
-	return call_int_hook(inode_need_killpriv, dentry);
+	int rc;
+	bool need = false;
+	struct security_hook_list *hp;
+
+	hlist_for_each_entry(hp, &security_hook_heads.inode_need_killpriv,
+			     list) {
+		rc = hp->hook.inode_need_killpriv(dentry, &need);
+		if (rc < 0)
+			return rc;
+		if (need)
+			break;
+	}
+
+	if (need)
+		*attr |= ATTR_KILL_PRIV;
+	else
+		*attr &= ~ATTR_KILL_PRIV;
+
+	return 0;
 }
 
 /**
-- 
2.30.2

[PATCH bpf-next v4 03/20] lsm: Refactor return value of LSM hook inode_getsecurity

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook inode_getsecurity to 0 or a negative error code.

Before:
- Hook inode_getsecurity returns size of buffer on success or a
  negative error code on failure.

After:
- Hook inode_getsecurity returns 0 on success or a negative error
  code on failure. An output parameter @len is introduced to hold
  the buffer size on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 fs/xattr.c                    | 19 ++++++++++---------
 include/linux/lsm_hook_defs.h |  3 ++-
 include/linux/security.h      | 12 ++++++------
 security/commoncap.c          |  9 ++++++---
 security/security.c           | 11 ++++++-----
 security/selinux/hooks.c      | 16 ++++++----------
 security/smack/smack_lsm.c    | 14 +++++++-------
 7 files changed, 43 insertions(+), 41 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index f8b643f91a98..f4e3bedf7272 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -339,27 +339,28 @@ xattr_getsecurity(struct mnt_idmap *idmap, struct inode *inode,
 		  const char *name, void *value, size_t size)
 {
 	void *buffer = NULL;
-	ssize_t len;
+	int error;
+	u32 len;
 
 	if (!value || !size) {
-		len = security_inode_getsecurity(idmap, inode, name,
-						 &buffer, false);
+		error = security_inode_getsecurity(idmap, inode, name,
+						   false, &buffer, &len);
 		goto out_noalloc;
 	}
 
-	len = security_inode_getsecurity(idmap, inode, name, &buffer,
-					 true);
-	if (len < 0)
-		return len;
+	error = security_inode_getsecurity(idmap, inode, name, true,
+					   &buffer, &len);
+	if (error)
+		return error;
 	if (size < len) {
-		len = -ERANGE;
+		error = -ERANGE;
 		goto out;
 	}
 	memcpy(value, buffer, len);
 out:
 	kfree(buffer);
 out_noalloc:
-	return len;
+	return error < 0 ? error : len;
 }
 
 /*
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 964849de424b..4f056f2613af 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -169,7 +169,8 @@ LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry, bool *need)
 LSM_HOOK(int, 0, inode_killpriv, struct mnt_idmap *idmap,
 	 struct dentry *dentry)
 LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap,
-	 struct inode *inode, const char *name, void **buffer, bool alloc)
+	 struct inode *inode, const char *name, bool alloc, void **buffer,
+	 u32 *len)
 LSM_HOOK(int, -EOPNOTSUPP, inode_setsecurity, struct inode *inode,
 	 const char *name, const void *value, size_t size, int flags)
 LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
diff --git a/include/linux/security.h b/include/linux/security.h
index 1614ef5b2dd2..b6d296d21438 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -164,8 +164,8 @@ int cap_inode_removexattr(struct mnt_idmap *idmap,
 int cap_inode_need_killpriv(struct dentry *dentry, bool *need);
 int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
 int cap_inode_getsecurity(struct mnt_idmap *idmap,
-			  struct inode *inode, const char *name, void **buffer,
-			  bool alloc);
+			  struct inode *inode, const char *name, bool alloc,
+			  void **buffer, u32 *len);
 extern int cap_mmap_addr(unsigned long addr);
 extern int cap_mmap_file(struct file *file, unsigned long reqprot,
 			 unsigned long prot, unsigned long flags);
@@ -393,7 +393,7 @@ int security_inode_need_killpriv(struct dentry *dentry, int *attr);
 int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
 int security_inode_getsecurity(struct mnt_idmap *idmap,
 			       struct inode *inode, const char *name,
-			       void **buffer, bool alloc);
+			       bool alloc, void **buffer, u32 *len);
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
@@ -996,10 +996,10 @@ static inline int security_inode_killpriv(struct mnt_idmap *idmap,
 
 static inline int security_inode_getsecurity(struct mnt_idmap *idmap,
 					     struct inode *inode,
-					     const char *name, void **buffer,
-					     bool alloc)
+					     const char *name, bool alloc,
+					     void **buffer, u32 *len)
 {
-	return cap_inode_getsecurity(idmap, inode, name, buffer, alloc);
+	return cap_inode_getsecurity(idmap, inode, name, alloc, buffer, len);
 }
 
 static inline int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
diff --git a/security/commoncap.c b/security/commoncap.c
index 17d6188d22cf..ff82e2ab6f8f 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -383,8 +383,8 @@ static bool is_v3header(int size, const struct vfs_cap_data *cap)
  * so that's good.
  */
 int cap_inode_getsecurity(struct mnt_idmap *idmap,
-			  struct inode *inode, const char *name, void **buffer,
-			  bool alloc)
+			  struct inode *inode, const char *name,
+			  bool alloc, void **buffer, u32 *len)
 {
 	int size;
 	kuid_t kroot;
@@ -485,7 +485,10 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
 	}
 out_free:
 	kfree(tmpbuf);
-	return size;
+	if (size < 0)
+		return size;
+	*len = size;
+	return 0;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index a4abcd86eb36..614f14cbfff7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2544,8 +2544,9 @@ int security_inode_killpriv(struct mnt_idmap *idmap,
  * @idmap: idmap of the mount
  * @inode: inode
  * @name: xattr name
- * @buffer: security label buffer
  * @alloc: allocation flag
+ * @buffer: security label buffer
+ * @len: security label length
  *
  * Retrieve a copy of the extended attribute representation of the security
  * label associated with @name for @inode via @buffer.  Note that @name is the
@@ -2553,17 +2554,17 @@ int security_inode_killpriv(struct mnt_idmap *idmap,
  * @alloc is used to specify if the call should return a value via the buffer
  * or just the value length.
  *
- * Return: Returns size of buffer on success.
+ * Return: Returns 0 on success or a negative error code on failure.
  */
 int security_inode_getsecurity(struct mnt_idmap *idmap,
 			       struct inode *inode, const char *name,
-			       void **buffer, bool alloc)
+			       bool alloc, void **buffer, u32 *len)
 {
 	if (unlikely(IS_PRIVATE(inode)))
 		return LSM_RET_DEFAULT(inode_getsecurity);
 
-	return call_int_hook(inode_getsecurity, idmap, inode, name, buffer,
-			     alloc);
+	return call_int_hook(inode_getsecurity, idmap, inode, name, alloc,
+			     buffer, len);
 }
 
 /**
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9cd5a8f1f6a3..70792bba24d9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3407,7 +3407,7 @@ static int selinux_path_notify(const struct path *path, u64 mask,
  */
 static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
 				     struct inode *inode, const char *name,
-				     void **buffer, bool alloc)
+				     bool alloc, void **buffer, u32 *len)
 {
 	u32 size;
 	int error;
@@ -3440,14 +3440,14 @@ static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
 						&context, &size);
 	if (error)
 		return error;
-	error = size;
+	*len = size;
 	if (alloc) {
 		*buffer = context;
 		goto out_nofree;
 	}
 	kfree(context);
 out_nofree:
-	return error;
+	return 0;
 }
 
 static int selinux_inode_setsecurity(struct inode *inode, const char *name,
@@ -6644,13 +6644,9 @@ static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
 
 static int selinux_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 {
-	int len = 0;
-	len = selinux_inode_getsecurity(&nop_mnt_idmap, inode,
-					XATTR_SELINUX_SUFFIX, ctx, true);
-	if (len < 0)
-		return len;
-	*ctxlen = len;
-	return 0;
+	return selinux_inode_getsecurity(&nop_mnt_idmap, inode,
+					 XATTR_SELINUX_SUFFIX,
+					 true, ctx, ctxlen);
 }
 #ifdef CONFIG_KEYS
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index f5cbec1e6a92..e7a5f6fd9a2d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1543,14 +1543,15 @@ static int smack_inode_remove_acl(struct mnt_idmap *idmap,
  * @idmap: idmap of the mount
  * @inode: the object
  * @name: attribute name
- * @buffer: where to put the result
  * @alloc: duplicate memory
+ * @buffer: where to put the result
+ * @len: where to put the result length
  *
- * Returns the size of the attribute or an error code
+ * Returns 0 on success or a negative error code on failure
  */
 static int smack_inode_getsecurity(struct mnt_idmap *idmap,
 				   struct inode *inode, const char *name,
-				   void **buffer, bool alloc)
+				   bool alloc, void **buffer, u32 *len)
 {
 	struct socket_smack *ssp;
 	struct socket *sock;
@@ -1558,7 +1559,6 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap,
 	struct inode *ip = inode;
 	struct smack_known *isp;
 	struct inode_smack *ispp;
-	size_t label_len;
 	char *label = NULL;
 
 	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
@@ -1594,15 +1594,15 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap,
 	if (!label)
 		label = isp->smk_known;
 
-	label_len = strlen(label);
-
 	if (alloc) {
 		*buffer = kstrdup(label, GFP_KERNEL);
 		if (*buffer == NULL)
 			return -ENOMEM;
 	}
 
-	return label_len;
+	*len = strlen(label);
+
+	return 0;
 }
 
 
-- 
2.30.2

[PATCH bpf-next v4 04/20] lsm: Refactor return value of LSM hook inode_listsecurity

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook inode_listsecurity to 0 or a negative error code.

Before:
- Hook inode_listsecurity returns number of bytes used/required on
  success or a negative error code on failure.

After:
- Hook inode_listsecurity returns 0 on success or a negative error
  code on failure. An output parameter @bytes is introduced to hold
  the number of bytes used/required on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 fs/nfs/nfs4proc.c             |  5 ++++-
 fs/xattr.c                    |  5 ++++-
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  7 ++++---
 net/socket.c                  |  9 +++++----
 security/security.c           | 29 +++++++++++++++++++++++++----
 security/selinux/hooks.c      |  8 +++++---
 security/smack/smack_lsm.c    |  6 ++++--
 8 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index a691fa10b3e9..6d75758ba3d5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7848,10 +7848,13 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
 static ssize_t
 nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len)
 {
+	size_t bytes;
 	int len = 0;
 
 	if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) {
-		len = security_inode_listsecurity(inode, list, list_len);
+		len = security_inode_listsecurity(inode, list, list_len, &bytes);
+		if (!len)
+			len = bytes;
 		if (len >= 0 && list_len && len > list_len)
 			return -ERANGE;
 	}
diff --git a/fs/xattr.c b/fs/xattr.c
index f4e3bedf7272..ab7d7123a016 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -485,6 +485,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
 {
 	struct inode *inode = d_inode(dentry);
 	ssize_t error;
+	size_t bytes;
 
 	error = security_inode_listxattr(dentry);
 	if (error)
@@ -493,7 +494,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
 	if (inode->i_op->listxattr) {
 		error = inode->i_op->listxattr(dentry, list, size);
 	} else {
-		error = security_inode_listsecurity(inode, list, size);
+		error = security_inode_listsecurity(inode, list, size, &bytes);
+		if (!error)
+			error = bytes;
 		if (size && error > size)
 			error = -ERANGE;
 	}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 4f056f2613af..1b7761ae2777 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -174,7 +174,7 @@ LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap,
 LSM_HOOK(int, -EOPNOTSUPP, inode_setsecurity, struct inode *inode,
 	 const char *name, const void *value, size_t size, int flags)
 LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
-	 size_t buffer_size)
+	 size_t buffer_size, size_t *bytes)
 LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
 LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
 LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, struct dentry *src,
diff --git a/include/linux/security.h b/include/linux/security.h
index b6d296d21438..0ed53e232c4d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -395,7 +395,7 @@ int security_inode_getsecurity(struct mnt_idmap *idmap,
 			       struct inode *inode, const char *name,
 			       bool alloc, void **buffer, u32 *len);
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
-int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
+int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size, size_t *bytes);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
 int security_inode_copy_up(struct dentry *src, struct cred **new);
 int security_inode_copy_up_xattr(struct dentry *src, const char *name);
@@ -1007,9 +1007,10 @@ static inline int security_inode_setsecurity(struct inode *inode, const char *na
 	return -EOPNOTSUPP;
 }
 
-static inline int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
+static inline int security_inode_listsecurity(struct inode *inode, char *buffer,
+					      size_t buffer_size, size_t *bytes)
 {
-	return 0;
+	return *bytes = 0;
 }
 
 static inline void security_inode_getsecid(struct inode *inode, u32 *secid)
diff --git a/net/socket.c b/net/socket.c
index e416920e9399..43f0e3c9a6e0 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -571,12 +571,13 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
 static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer,
 				size_t size)
 {
-	ssize_t len;
+	int err;
+	size_t len;
 	ssize_t used = 0;
 
-	len = security_inode_listsecurity(d_inode(dentry), buffer, size);
-	if (len < 0)
-		return len;
+	err = security_inode_listsecurity(d_inode(dentry), buffer, size, &len);
+	if (err < 0)
+		return err;
 	used += len;
 	if (buffer) {
 		if (size < used)
diff --git a/security/security.c b/security/security.c
index 614f14cbfff7..26eea8f4cd74 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2597,20 +2597,41 @@ int security_inode_setsecurity(struct inode *inode, const char *name,
  * @inode: inode
  * @buffer: buffer
  * @buffer_size: size of buffer
+ * @bytes: number of bytes used/required
  *
  * Copy the extended attribute names for the security labels associated with
  * @inode into @buffer.  The maximum size of @buffer is specified by
  * @buffer_size.  @buffer may be NULL to request the size of the buffer
  * required.
  *
- * Return: Returns number of bytes used/required on success.
+ * Return: Returns 0 on success or a negative error code on failure.
  */
 int security_inode_listsecurity(struct inode *inode,
-				char *buffer, size_t buffer_size)
+				char *buffer, size_t buffer_size,
+				size_t *bytes)
 {
+	int rc;
+	size_t used;
+	struct security_hook_list *hp;
+
 	if (unlikely(IS_PRIVATE(inode)))
-		return 0;
-	return call_int_hook(inode_listsecurity, inode, buffer, buffer_size);
+		return *bytes = 0;
+
+	used = 0;
+	hlist_for_each_entry(hp, &security_hook_heads.inode_listsecurity,
+			     list) {
+		rc = hp->hook.inode_listsecurity(inode, buffer, buffer_size,
+						 &used);
+		if (rc < 0)
+			return rc;
+		if (used != 0)
+			break;
+	}
+
+	*bytes = used;
+
+	return 0;
+
 }
 EXPORT_SYMBOL(security_inode_listsecurity);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 70792bba24d9..5dedd3917d57 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3481,16 +3481,18 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
 	return 0;
 }
 
-static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
+static int selinux_inode_listsecurity(struct inode *inode, char *buffer,
+				      size_t buffer_size, size_t *bytes)
 {
 	const int len = sizeof(XATTR_NAME_SELINUX);
 
 	if (!selinux_initialized())
-		return 0;
+		return *bytes = 0;
 
 	if (buffer && len <= buffer_size)
 		memcpy(buffer, XATTR_NAME_SELINUX, len);
-	return len;
+	*bytes = len;
+	return 0;
 }
 
 static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index e7a5f6fd9a2d..6f73906bf7ea 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1611,16 +1611,18 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap,
  * @inode: the object
  * @buffer: where they go
  * @buffer_size: size of buffer
+ * @bytes: number of data bytes in buffer
  */
 static int smack_inode_listsecurity(struct inode *inode, char *buffer,
-				    size_t buffer_size)
+				    size_t buffer_size, size_t *bytes)
 {
 	int len = sizeof(XATTR_NAME_SMACK);
 
 	if (buffer != NULL && len <= buffer_size)
 		memcpy(buffer, XATTR_NAME_SMACK, len);
 
-	return len;
+	*bytes = len;
+	return 0;
 }
 
 /**
-- 
2.30.2

[PATCH bpf-next v4 05/20] lsm: Refactor return value of LSM hook inode_copy_up_xattr

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook inode_copy_up_xattr to 0 or a negative error code.

Before:
- Hook inode_copy_up_xattr returns 0 when accepting xattr, 1 when
  discarding xattr, -EOPNOTSUPP if it does not know xattr, or any
  other negative error code otherwise.

After:
- Hook inode_copy_up_xattr returns 0 when accepting xattr, *-ECANCELED*
  when discarding xattr, -EOPNOTSUPP if it does not know xattr, or
  any other negative error code otherwise.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 fs/overlayfs/copy_up.c            |  6 +++---
 security/integrity/evm/evm_main.c |  2 +-
 security/security.c               | 12 ++++++------
 security/selinux/hooks.c          |  4 ++--
 security/smack/smack_lsm.c        |  6 +++---
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index a5ef2005a2cc..337a5be99ac9 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -115,12 +115,12 @@ int ovl_copy_xattr(struct super_block *sb, const struct path *oldpath, struct de
 			continue;
 
 		error = security_inode_copy_up_xattr(old, name);
-		if (error < 0 && error != -EOPNOTSUPP)
-			break;
-		if (error == 1) {
+		if (error == -ECANCELED) {
 			error = 0;
 			continue; /* Discard */
 		}
+		if (error < 0 && error != -EOPNOTSUPP)
+			break;
 
 		if (is_posix_acl_xattr(name)) {
 			error = ovl_copy_acl(OVL_FS(sb), oldpath, new, name);
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 62fe66dd53ce..6924ed508ebd 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -1000,7 +1000,7 @@ static int evm_inode_copy_up_xattr(struct dentry *src, const char *name)
 	case EVM_XATTR_HMAC:
 	case EVM_IMA_XATTR_DIGSIG:
 	default:
-		rc = 1; /* discard */
+		rc = -ECANCELED; /* discard */
 	}
 
 	kfree(xattr_data);
diff --git a/security/security.c b/security/security.c
index 26eea8f4cd74..12215ca286af 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2675,18 +2675,18 @@ EXPORT_SYMBOL(security_inode_copy_up);
  * lower layer to the union/overlay layer.   The caller is responsible for
  * reading and writing the xattrs, this hook is merely a filter.
  *
- * Return: Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP
- *         if the security module does not know about attribute, or a negative
- *         error code to abort the copy up.
+ * Return: Returns 0 to accept the xattr, -ECANCELED to discard the xattr,
+ *         -EOPNOTSUPP if the security module does not know about attribute,
+ *         or a negative error code to abort the copy up.
  */
 int security_inode_copy_up_xattr(struct dentry *src, const char *name)
 {
 	int rc;
 
 	/*
-	 * The implementation can return 0 (accept the xattr), 1 (discard the
-	 * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
-	 * any other error code in case of an error.
+	 * The implementation can return 0 (accept the xattr), -ECANCELED
+	 * (discard the xattr), -EOPNOTSUPP if it does not know anything
+	 *  about the xattr or any other error code in case of an error.
 	 */
 	rc = call_int_hook(inode_copy_up_xattr, src, name);
 	if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5dedd3917d57..f9a6637dfd78 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3528,8 +3528,8 @@ static int selinux_inode_copy_up_xattr(struct dentry *dentry, const char *name)
 	 * xattrs up.  Instead, filter out SELinux-related xattrs following
 	 * policy load.
 	 */
-	if (selinux_initialized() && strcmp(name, XATTR_NAME_SELINUX) == 0)
-		return 1; /* Discard */
+	if (selinux_initialized() && !strcmp(name, XATTR_NAME_SELINUX))
+		return -ECANCELED; /* Discard */
 	/*
 	 * Any other attribute apart from SELINUX is not claimed, supported
 	 * by selinux.
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6f73906bf7ea..ae8f1c2d0ca6 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4893,10 +4893,10 @@ static int smack_inode_copy_up(struct dentry *dentry, struct cred **new)
 static int smack_inode_copy_up_xattr(struct dentry *src, const char *name)
 {
 	/*
-	 * Return 1 if this is the smack access Smack attribute.
+	 * Return -ECANCELED if this is the smack access Smack attribute.
 	 */
-	if (strcmp(name, XATTR_NAME_SMACK) == 0)
-		return 1;
+	if (!strcmp(name, XATTR_NAME_SMACK))
+		return -ECANCELED;
 
 	return -EOPNOTSUPP;
 }
-- 
2.30.2

[PATCH bpf-next v4 06/20] lsm: Refactor return value of LSM hook getselfattr

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook getselfattr to 0 or a negative error code.

Before:
- Hook getselfattr returns number of attributes found on success
  or a negative error code on failure.

After:
- Hook getselfattr returns 0 on success or a negative error code
  on failure. An output parameter @nattr is introduced to hold
  the number of attributes found on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  5 +++--
 security/apparmor/lsm.c       |  5 +++--
 security/lsm_syscalls.c       |  6 +++++-
 security/security.c           | 18 +++++++++++-------
 security/selinux/hooks.c      | 13 +++++++++----
 security/smack/smack_lsm.c    | 13 +++++++++----
 7 files changed, 41 insertions(+), 21 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 1b7761ae2777..dbc16f14f42f 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -282,7 +282,7 @@ LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
 LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
 	 struct inode *inode)
 LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int attr,
-	 struct lsm_ctx __user *ctx, u32 *size, u32 flags)
+	 struct lsm_ctx __user *ctx, u32 *size, u32 flags, u32 *nattr)
 LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
 	 struct lsm_ctx *ctx, u32 size, u32 flags)
 LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
diff --git a/include/linux/security.h b/include/linux/security.h
index 0ed53e232c4d..96a63e132abf 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -491,7 +491,7 @@ int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
 			unsigned nsops, int alter);
 void security_d_instantiate(struct dentry *dentry, struct inode *inode);
 int security_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
-			 u32 __user *size, u32 flags);
+			 u32 __user *size, u32 flags, u32 *nattr);
 int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 			 u32 size, u32 flags);
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
@@ -1420,7 +1420,8 @@ static inline void security_d_instantiate(struct dentry *dentry,
 
 static inline int security_getselfattr(unsigned int attr,
 				       struct lsm_ctx __user *ctx,
-				       size_t __user *size, u32 flags)
+				       size_t __user *size, u32 flags,
+				       u32 *nattr)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 6239777090c4..72dd09993f28 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -779,7 +779,7 @@ static int apparmor_sb_pivotroot(const struct path *old_path,
 }
 
 static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
-				u32 *size, u32 flags)
+				u32 *size, u32 flags, u32 *nattr)
 {
 	int error = -ENOENT;
 	struct aa_task_ctx *ctx = task_ctx(current);
@@ -815,7 +815,8 @@ static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
 
 	if (error < 0)
 		return error;
-	return 1;
+	*nattr = 1;
+	return 0;
 }
 
 static int apparmor_getprocattr(struct task_struct *task, const char *name,
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index 8440948a690c..845866f94b03 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c
@@ -77,7 +77,11 @@ SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
 SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
 		ctx, u32 __user *, size, u32, flags)
 {
-	return security_getselfattr(attr, ctx, size, flags);
+	int rc;
+	u32 nattr;
+
+	rc = security_getselfattr(attr, ctx, size, flags, &nattr);
+	return rc < 0 ? rc : nattr;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index 12215ca286af..095e78efcb32 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3969,21 +3969,23 @@ EXPORT_SYMBOL(security_d_instantiate);
  * @flags: special handling options. LSM_FLAG_SINGLE indicates that only
  * attributes associated with the LSM identified in the passed @ctx be
  * reported.
+ * @nattr: number of attributes found on success
  *
  * A NULL value for @uctx can be used to get both the number of attributes
  * and the size of the data.
  *
- * Returns the number of attributes found on success, negative value
- * on error. @size is reset to the total size of the data.
- * If @size is insufficient to contain the data -E2BIG is returned.
+ * Returns 0 on success, a negative error code on failure. @size is reset
+ * to the total size of the data. If @size is insufficient to contain the
+ * data -E2BIG is returned.
  */
 int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
-			 u32 __user *size, u32 flags)
+			 u32 __user *size, u32 flags, u32 *nattr)
 {
 	struct security_hook_list *hp;
 	struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
 	u8 __user *base = (u8 __user *)uctx;
 	u32 entrysize;
+	u32 entrycount;
 	u32 total = 0;
 	u32 left;
 	bool toobig = false;
@@ -4024,7 +4026,8 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 		entrysize = left;
 		if (base)
 			uctx = (struct lsm_ctx __user *)(base + total);
-		rc = hp->hook.getselfattr(attr, uctx, &entrysize, flags);
+		rc = hp->hook.getselfattr(attr, uctx, &entrysize, flags,
+					  &entrycount);
 		if (rc == -EOPNOTSUPP) {
 			rc = 0;
 			continue;
@@ -4039,7 +4042,7 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 			left -= entrysize;
 
 		total += entrysize;
-		count += rc;
+		count += entrycount;
 		if (single)
 			break;
 	}
@@ -4047,9 +4050,10 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 		return -EFAULT;
 	if (toobig)
 		return -E2BIG;
+	*nattr = count;
 	if (count == 0)
 		return LSM_RET_DEFAULT(getselfattr);
-	return count;
+	return 0;
 }
 
 /*
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f9a6637dfd78..0d35bb93baca 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6536,15 +6536,17 @@ static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
  * @ctx: buffer to receive the result
  * @size: buffer size (input), buffer size used (output)
  * @flags: unused
+ * @nattr: number of attributes found on success.
  *
  * Fill the passed user space @ctx with the details of the requested
  * attribute.
  *
- * Returns the number of attributes on success, an error code otherwise.
- * There will only ever be one attribute.
+ * Returns 0 on success or a negative error code on failure.
+ * There will only ever be one attribute, so @nattr is set to
+ * 1 on success.
  */
 static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
-			       u32 *size, u32 flags)
+			       u32 *size, u32 flags, u32 *nattr)
 {
 	int rc;
 	char *val = NULL;
@@ -6555,7 +6557,10 @@ static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 		return val_len;
 	rc = lsm_fill_user_ctx(ctx, size, val, val_len, LSM_ID_SELINUX, 0);
 	kfree(val);
-	return (!rc ? 1 : rc);
+	if (rc < 0)
+		return rc;
+	*nattr = 1;
+	return 0;
 }
 
 static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ae8f1c2d0ca6..63d9c5f456c1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3648,15 +3648,17 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
  * @ctx: buffer to receive the result
  * @size: available size in, actual size out
  * @flags: unused
+ * @nattr: number of attributes found on success
  *
  * Fill the passed user space @ctx with the details of the requested
  * attribute.
  *
- * Returns the number of attributes on success, an error code otherwise.
- * There will only ever be one attribute.
+ * Returns 0 on success or a ngetaive error code on failure.
+ * There will only ever be one attribute, so @nattr is set to
+ * 1 on success.
  */
 static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
-			     u32 *size, u32 flags)
+			     u32 *size, u32 flags, u32 *nattr)
 {
 	int rc;
 	struct smack_known *skp;
@@ -3668,7 +3670,10 @@ static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 	rc = lsm_fill_user_ctx(ctx, size,
 			       skp->smk_known, strlen(skp->smk_known) + 1,
 			       LSM_ID_SMACK, 0);
-	return (!rc ? 1 : rc);
+	if (rc < 0)
+		return rc;
+	*nattr = 1;
+	return 0;
 }
 
 /**
-- 
2.30.2

[PATCH bpf-next v4 07/20] lsm: Refactor return value of LSM hook setprocattr

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook setprocattr to 0 or a negative error code.

Before:
- Hook setprocattr returns the number of bytes written on success
  or a negative error code on failure.

After:
- Hook setprocattr returns 0 on success or a negative error code
  on failure. An output parameter @wbytes is introduced to hold
  the number of bytes written on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 fs/proc/base.c                |  5 +++--
 include/linux/lsm_hook_defs.h |  3 ++-
 include/linux/security.h      |  5 +++--
 security/apparmor/lsm.c       | 10 +++++++---
 security/security.c           |  8 +++++---
 security/selinux/hooks.c      | 11 ++++++++---
 security/smack/smack_lsm.c    | 14 ++++++++++----
 7 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 72a1acd03675..9e1cf6cc674d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2740,6 +2740,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 {
 	struct inode * inode = file_inode(file);
 	struct task_struct *task;
+	size_t wbytes;
 	void *page;
 	int rv;
 
@@ -2785,12 +2786,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
 
 	rv = security_setprocattr(PROC_I(inode)->op.lsmid,
 				  file->f_path.dentry->d_name.name, page,
-				  count);
+				  count, &wbytes);
 	mutex_unlock(&current->signal->cred_guard_mutex);
 out_free:
 	kfree(page);
 out:
-	return rv;
+	return rv < 0 ? rv : wbytes;
 }
 
 static const struct file_operations proc_pid_attr_operations = {
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index dbc16f14f42f..2628514bb19c 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -287,7 +287,8 @@ LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
 	 struct lsm_ctx *ctx, u32 size, u32 flags)
 LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
 	 char **value)
-LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
+LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size,
+	 size_t *wbytes)
 LSM_HOOK(int, 0, ismaclabel, const char *name)
 LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata,
 	 u32 *seclen)
diff --git a/include/linux/security.h b/include/linux/security.h
index 96a63e132abf..1f1a9696e65d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -496,7 +496,8 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 			 u32 size, u32 flags);
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
 			 char **value);
-int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
+int security_setprocattr(int lsmid, const char *name, void *value, size_t size,
+			 size_t *wbytes);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
 int security_ismaclabel(const char *name);
 int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
@@ -1440,7 +1441,7 @@ static inline int security_getprocattr(struct task_struct *p, int lsmid,
 }
 
 static inline int security_setprocattr(int lsmid, char *name, void *value,
-				       size_t size)
+				       size_t size, size_t *wbytes)
 {
 	return -EINVAL;
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 72dd09993f28..6c8b1f8c5781 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -939,13 +939,17 @@ static int apparmor_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
 }
 
 static int apparmor_setprocattr(const char *name, void *value,
-				size_t size)
+				size_t size, size_t *wbytes)
 {
+	int rc = -EINVAL;
 	int attr = lsm_name_to_attr(name);
 
 	if (attr)
-		return do_setattr(attr, value, size);
-	return -EINVAL;
+		rc = do_setattr(attr, value, size);
+	if (rc < 0)
+		return rc;
+	*wbytes = rc;
+	return 0;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index 095e78efcb32..9685096dbf16 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4141,20 +4141,22 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
  * @name: attribute name
  * @value: attribute value
  * @size: attribute value size
+ * @wbytes: bytes written on success
  *
  * Write (set) the current task's attribute @name to @value, size @size if
  * allowed.
  *
- * Return: Returns bytes written on success, a negative value otherwise.
+ * Return: Returns 0 on success, a negative error code otherwise.
  */
-int security_setprocattr(int lsmid, const char *name, void *value, size_t size)
+int security_setprocattr(int lsmid, const char *name, void *value, size_t size,
+			 size_t *wbytes)
 {
 	struct security_hook_list *hp;
 
 	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
 		if (lsmid != 0 && lsmid != hp->lsmid->id)
 			continue;
-		return hp->hook.setprocattr(name, value, size);
+		return hp->hook.setprocattr(name, value, size, wbytes);
 	}
 	return LSM_RET_DEFAULT(setprocattr);
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0d35bb93baca..7a73f3710025 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6589,13 +6589,18 @@ static int selinux_getprocattr(struct task_struct *p,
 	return -EINVAL;
 }
 
-static int selinux_setprocattr(const char *name, void *value, size_t size)
+static int selinux_setprocattr(const char *name, void *value, size_t size,
+			       size_t *wbytes)
 {
+	int rc = -EINVAL;
 	int attr = lsm_name_to_attr(name);
 
 	if (attr)
-		return selinux_lsm_setattr(attr, value, size);
-	return -EINVAL;
+		rc = selinux_lsm_setattr(attr, value, size);
+	if (rc < 0)
+		return rc;
+	*wbytes = rc;
+	return 0;
 }
 
 static int selinux_ismaclabel(const char *name)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 63d9c5f456c1..4265f2639106 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3797,19 +3797,25 @@ static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
  * @name: the name of the attribute in /proc/.../attr
  * @value: the value to set
  * @size: the size of the value
+ * @wbytes: the length of the smack label written
  *
  * Sets the Smack value of the task. Only setting self
  * is permitted and only with privilege
  *
- * Returns the length of the smack label or an error code
+ * Returns 0 on success or a negative error code
  */
-static int smack_setprocattr(const char *name, void *value, size_t size)
+static int smack_setprocattr(const char *name, void *value, size_t size,
+			     size_t *wbytes)
 {
+	int rc = -EINVAL;
 	int attr = lsm_name_to_attr(name);
 
 	if (attr != LSM_ATTR_UNDEF)
-		return do_setattr(attr, value, size);
-	return -EINVAL;
+		rc = do_setattr(attr, value, size);
+	if (rc < 0)
+		return rc;
+	*wbytes = rc;
+	return 0;
 }
 
 /**
-- 
2.30.2

[PATCH bpf-next v4 08/20] lsm: Refactor return value of LSM hook getprocattr

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook getprocattr to 0 or a negative error code.

Before:
- Hook getprocattr returns length of value on success or a negative
  error code on failure.

After:
- Hook getprocattr returns 0 on success or a negative error code on
  failure. An output parameter @len is introduced to hold the
  length of value on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 fs/proc/base.c                |  5 ++++-
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  5 +++--
 security/apparmor/lsm.c       |  7 +++++--
 security/security.c           |  8 +++++---
 security/selinux/hooks.c      | 16 +++++++++-------
 security/smack/smack_lsm.c    | 11 ++++++-----
 7 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9e1cf6cc674d..516a00f6ce36 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2721,13 +2721,16 @@ static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,
 	char *p = NULL;
 	ssize_t length;
 	struct task_struct *task = get_proc_task(inode);
+	u32 n;
 
 	if (!task)
 		return -ESRCH;
 
 	length = security_getprocattr(task, PROC_I(inode)->op.lsmid,
 				      file->f_path.dentry->d_name.name,
-				      &p);
+				      &p, &n);
+	if (!length)
+		length = n;
 	put_task_struct(task);
 	if (length > 0)
 		length = simple_read_from_buffer(buf, count, ppos, p, length);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2628514bb19c..b0e3cf3fc33f 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -286,7 +286,7 @@ LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int attr,
 LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
 	 struct lsm_ctx *ctx, u32 size, u32 flags)
 LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
-	 char **value)
+	 char **value, u32 *len)
 LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size,
 	 size_t *wbytes)
 LSM_HOOK(int, 0, ismaclabel, const char *name)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1f1a9696e65d..616047030a89 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -495,7 +495,7 @@ int security_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 			 u32 size, u32 flags);
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
-			 char **value);
+			 char **value, u32 *len);
 int security_setprocattr(int lsmid, const char *name, void *value, size_t size,
 			 size_t *wbytes);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
@@ -1435,7 +1435,8 @@ static inline int security_setselfattr(unsigned int attr,
 }
 
 static inline int security_getprocattr(struct task_struct *p, int lsmid,
-				       const char *name, char **value)
+				       const char *name, char **value,
+				       u32 *len)
 {
 	return -EINVAL;
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 6c8b1f8c5781..0454f3f1af06 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -820,7 +820,7 @@ static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
 }
 
 static int apparmor_getprocattr(struct task_struct *task, const char *name,
-				char **value)
+				char **value, u32 *len)
 {
 	int error = -ENOENT;
 	/* released below */
@@ -843,7 +843,10 @@ static int apparmor_getprocattr(struct task_struct *task, const char *name,
 	aa_put_label(label);
 	put_cred(cred);
 
-	return error;
+	if (error < 0)
+		return error;
+	*len = error;
+	return 0;
 }
 
 static int do_setattr(u64 attr, void *value, size_t size)
diff --git a/security/security.c b/security/security.c
index 9685096dbf16..9dd2ae6cf763 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4117,20 +4117,22 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
  * @lsmid: LSM identification
  * @name: attribute name
  * @value: attribute value
+ * @len: length of @value
  *
  * Read attribute @name for task @p and store it into @value if allowed.
  *
- * Return: Returns the length of @value on success, a negative value otherwise.
+ * Return: Returns 0 on success or a negative error code on failure.
+ *         @len is set to the length of @value on success.
  */
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
-			 char **value)
+			 char **value, u32 *len)
 {
 	struct security_hook_list *hp;
 
 	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
 		if (lsmid != 0 && lsmid != hp->lsmid->id)
 			continue;
-		return hp->hook.getprocattr(p, name, value);
+		return hp->hook.getprocattr(p, name, value, len);
 	}
 	return LSM_RET_DEFAULT(getprocattr);
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7a73f3710025..16cd336aab3d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6574,19 +6574,21 @@ static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
 	return rc;
 }
 
-static int selinux_getprocattr(struct task_struct *p,
-			       const char *name, char **value)
+static int selinux_getprocattr(struct task_struct *p, const char *name,
+			       char **value, u32 *len)
 {
 	unsigned int attr = lsm_name_to_attr(name);
-	int rc;
+	int rc = -EINVAL;
 
 	if (attr) {
 		rc = selinux_lsm_getattr(attr, p, value);
-		if (rc != -EOPNOTSUPP)
-			return rc;
+		if (rc == -EOPNOTSUPP)
+			rc = -EINVAL;
 	}
-
-	return -EINVAL;
+	if (rc < 0)
+		return rc;
+	*len = rc;
+	return 0;
 }
 
 static int selinux_setprocattr(const char *name, void *value, size_t size,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 4265f2639106..8a352bd05565 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3681,16 +3681,17 @@ static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
  * @p: the object task
  * @name: the name of the attribute in /proc/.../attr
  * @value: where to put the result
+ * @len: where to put the length of the result
  *
  * Places a copy of the task Smack into value
  *
- * Returns the length of the smack label or an error code
+ * Returns 0 on success or a negative error code on failure.
  */
-static int smack_getprocattr(struct task_struct *p, const char *name, char **value)
+static int smack_getprocattr(struct task_struct *p, const char *name,
+			     char **value, u32 *len)
 {
 	struct smack_known *skp = smk_of_task_struct_obj(p);
 	char *cp;
-	int slen;
 
 	if (strcmp(name, "current") != 0)
 		return -EINVAL;
@@ -3699,9 +3700,9 @@ static int smack_getprocattr(struct task_struct *p, const char *name, char **val
 	if (cp == NULL)
 		return -ENOMEM;
 
-	slen = strlen(cp);
+	*len = strlen(cp);
 	*value = cp;
-	return slen;
+	return 0;
 }
 
 /**
-- 
2.30.2

[PATCH bpf-next v4 09/20] lsm: Refactor return value of LSM hook key_getsecurity

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook key_getsecurity to 0 or a negative error code.

Before:
- Hook key_getsecurity returns length of value on success or a
  negative error code on failure.

After:
- Hook key_getsecurity returns 0 on success or a negative error
  code on failure. An output parameter @len is introduced to hold
  the length of value on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/lsm_hook_defs.h |  3 ++-
 include/linux/security.h      |  6 ++++--
 security/keys/keyctl.c        | 11 ++++++++---
 security/security.c           | 26 +++++++++++++++++++++-----
 security/selinux/hooks.c      | 11 +++++------
 security/smack/smack_lsm.c    | 21 +++++++++++----------
 6 files changed, 51 insertions(+), 27 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index b0e3cf3fc33f..54fec360947c 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -407,7 +407,8 @@ LSM_HOOK(int, 0, key_alloc, struct key *key, const struct cred *cred,
 LSM_HOOK(void, LSM_RET_VOID, key_free, struct key *key)
 LSM_HOOK(int, 0, key_permission, key_ref_t key_ref, const struct cred *cred,
 	 enum key_need_perm need_perm)
-LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **buffer)
+LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **buffer,
+	 size_t *len)
 LSM_HOOK(void, LSM_RET_VOID, key_post_create_or_update, struct key *keyring,
 	 struct key *key, const void *payload, size_t payload_len,
 	 unsigned long flags, bool create)
diff --git a/include/linux/security.h b/include/linux/security.h
index 616047030a89..4e64e51a1a86 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2020,7 +2020,7 @@ int security_key_alloc(struct key *key, const struct cred *cred, unsigned long f
 void security_key_free(struct key *key);
 int security_key_permission(key_ref_t key_ref, const struct cred *cred,
 			    enum key_need_perm need_perm);
-int security_key_getsecurity(struct key *key, char **_buffer);
+int security_key_getsecurity(struct key *key, char **_buffer, size_t *_len);
 void security_key_post_create_or_update(struct key *keyring, struct key *key,
 					const void *payload, size_t payload_len,
 					unsigned long flags, bool create);
@@ -2045,9 +2045,11 @@ static inline int security_key_permission(key_ref_t key_ref,
 	return 0;
 }
 
-static inline int security_key_getsecurity(struct key *key, char **_buffer)
+static inline int security_key_getsecurity(struct key *key, char **_buffer,
+					   size_t *_len)
 {
 	*_buffer = NULL;
+	*_len = 0;
 	return 0;
 }
 
diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
index 4bc3e9398ee3..e9f857620f28 100644
--- a/security/keys/keyctl.c
+++ b/security/keys/keyctl.c
@@ -1565,6 +1565,7 @@ long keyctl_get_security(key_serial_t keyid,
 	struct key *key, *instkey;
 	key_ref_t key_ref;
 	char *context;
+	size_t len;
 	long ret;
 
 	key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
@@ -1586,15 +1587,18 @@ long keyctl_get_security(key_serial_t keyid,
 	}
 
 	key = key_ref_to_ptr(key_ref);
-	ret = security_key_getsecurity(key, &context);
-	if (ret == 0) {
+	ret = security_key_getsecurity(key, &context, &len);
+	if (ret < 0)
+		goto error;
+	if (len == 0) {
 		/* if no information was returned, give userspace an empty
 		 * string */
 		ret = 1;
 		if (buffer && buflen > 0 &&
 		    copy_to_user(buffer, "", 1) != 0)
 			ret = -EFAULT;
-	} else if (ret > 0) {
+	} else {
+		ret = len;
 		/* return as much data as there's room for */
 		if (buffer && buflen > 0) {
 			if (buflen > ret)
@@ -1607,6 +1611,7 @@ long keyctl_get_security(key_serial_t keyid,
 		kfree(context);
 	}
 
+error:
 	key_ref_put(key_ref);
 	return ret;
 }
diff --git a/security/security.c b/security/security.c
index 9dd2ae6cf763..2c161101074d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
  * security_key_getsecurity() - Get the key's security label
  * @key: key
  * @buffer: security label buffer
+ * @len: the length of @buffer (including terminating NULL) on success
  *
  * Get a textual representation of the security context attached to a key for
  * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
  * storage for the NUL-terminated string and the caller should free it.
  *
- * Return: Returns the length of @buffer (including terminating NUL) or -ve if
- *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
- *         there is no security label assigned to the key.
+ * Return: Returns 0 on success or -ve if an error occurs. May also return 0
+ *         (and a NULL buffer pointer) if there is no security label assigned
+ *         to the key.
  */
-int security_key_getsecurity(struct key *key, char **buffer)
+int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
 {
+	int rc;
+	size_t n = 0;
+	struct security_hook_list *hp;
+
 	*buffer = NULL;
-	return call_int_hook(key_getsecurity, key, buffer);
+
+	hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
+		rc = hp->hook.key_getsecurity(key, buffer, &n);
+		if (rc < 0)
+			return rc;
+		if (n)
+			break;
+	}
+
+	*len = n;
+
+	return 0;
 }
 
 /**
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 16cd336aab3d..747ec602dec0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6737,18 +6737,17 @@ static int selinux_key_permission(key_ref_t key_ref,
 	return avc_has_perm(sid, ksec->sid, SECCLASS_KEY, perm, NULL);
 }
 
-static int selinux_key_getsecurity(struct key *key, char **_buffer)
+static int selinux_key_getsecurity(struct key *key, char **_buffer,
+				   size_t *_len)
 {
 	struct key_security_struct *ksec = key->security;
 	char *context = NULL;
-	unsigned len;
+	u32 context_len;
 	int rc;
 
-	rc = security_sid_to_context(ksec->sid,
-				     &context, &len);
-	if (!rc)
-		rc = len;
+	rc = security_sid_to_context(ksec->sid, &context, &context_len);
 	*_buffer = context;
+	*_len = context_len;
 	return rc;
 }
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8a352bd05565..9a121ad53b16 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4585,30 +4585,31 @@ static int smack_key_permission(key_ref_t key_ref,
 /*
  * smack_key_getsecurity - Smack label tagging the key
  * @key points to the key to be queried
- * @_buffer points to a pointer that should be set to point to the
- * resulting string (if no label or an error occurs).
- * Return the length of the string (including terminating NUL) or -ve if
- * an error.
- * May also return 0 (and a NULL buffer pointer) if there is no label.
+ * @_buffer points to a pointer that should be set to point to the resulting
+ *          string (if no label or an error occurs).
+ * @_len  the length of the @_buffer (including terminating NUL)
+ *
+ * Return 0 on success or -ve if an error.
+ * If there is no label, @_buffer will be set to NULL and @_len will be set to
+ * 0.
  */
-static int smack_key_getsecurity(struct key *key, char **_buffer)
+static int smack_key_getsecurity(struct key *key, char **_buffer, size_t *_len)
 {
 	struct smack_known *skp = key->security;
-	size_t length;
 	char *copy;
 
 	if (key->security == NULL) {
 		*_buffer = NULL;
+		*_len = 0;
 		return 0;
 	}
 
 	copy = kstrdup(skp->smk_known, GFP_KERNEL);
 	if (copy == NULL)
 		return -ENOMEM;
-	length = strlen(copy) + 1;
-
+	*_len = strlen(copy) + 1;
 	*_buffer = copy;
-	return length;
+	return 0;
 }
 
 
-- 
2.30.2

[PATCH bpf-next v4 10/20] lsm: Refactor return value of LSM hook audit_rule_match

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

To be consistent with most LSM hooks, convert the return value of
hook audit_rule_match to 0 or a negative error code.

Before:
- Hook audit_rule_match returns 1 if the rule matches, 0 if it not,
  and negative error code otherwise.

After:
- Hook audit_rule_match returns 0 on success or a negative error
  code on failure. An output parameter @match is introduced to hold
  the match result on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/lsm_hook_defs.h     |  3 +-
 security/apparmor/audit.c         | 22 ++++++-------
 security/apparmor/include/audit.h |  2 +-
 security/security.c               | 15 ++++++++-
 security/selinux/include/audit.h  |  8 +++--
 security/selinux/ss/services.c    | 54 +++++++++++++++++--------------
 security/smack/smack_lsm.c        | 19 +++++++----
 7 files changed, 75 insertions(+), 48 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 54fec360947c..6b521744a23b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -418,7 +418,8 @@ LSM_HOOK(void, LSM_RET_VOID, key_post_create_or_update, struct key *keyring,
 LSM_HOOK(int, 0, audit_rule_init, u32 field, u32 op, char *rulestr,
 	 void **lsmrule, gfp_t gfp)
 LSM_HOOK(int, 0, audit_rule_known, struct audit_krule *krule)
-LSM_HOOK(int, 0, audit_rule_match, u32 secid, u32 field, u32 op, void *lsmrule)
+LSM_HOOK(int, 0, audit_rule_match, u32 secid, u32 field, u32 op, void *lsmrule,
+	 bool *match)
 LSM_HOOK(void, LSM_RET_VOID, audit_rule_free, void *lsmrule)
 #endif /* CONFIG_AUDIT */
 
diff --git a/security/apparmor/audit.c b/security/apparmor/audit.c
index 6b5181c668b5..352a183b3325 100644
--- a/security/apparmor/audit.c
+++ b/security/apparmor/audit.c
@@ -264,11 +264,11 @@ int aa_audit_rule_known(struct audit_krule *rule)
 	return 0;
 }
 
-int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
+int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, bool *match)
 {
 	struct aa_audit_rule *rule = vrule;
 	struct aa_label *label;
-	int found = 0;
+	bool found = false;
 
 	label = aa_secid_to_label(sid);
 
@@ -276,16 +276,14 @@ int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 		return -ENOENT;
 
 	if (aa_label_is_subset(label, rule->label))
-		found = 1;
+		found = true;
+
+	if (field == AUDIT_SUBJ_ROLE && op == Audit_equal)
+		*match = found;
+	else if (field == AUDIT_SUBJ_ROLE && op == Audit_not_equal)
+		*match = !found;
+	else
+		*match = false;
 
-	switch (field) {
-	case AUDIT_SUBJ_ROLE:
-		switch (op) {
-		case Audit_equal:
-			return found;
-		case Audit_not_equal:
-			return !found;
-		}
-	}
 	return 0;
 }
diff --git a/security/apparmor/include/audit.h b/security/apparmor/include/audit.h
index 0c8cc86b417b..a227741f33c8 100644
--- a/security/apparmor/include/audit.h
+++ b/security/apparmor/include/audit.h
@@ -202,6 +202,6 @@ static inline int complain_error(int error)
 void aa_audit_rule_free(void *vrule);
 int aa_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule, gfp_t gfp);
 int aa_audit_rule_known(struct audit_krule *rule);
-int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule);
+int aa_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, bool *match);
 
 #endif /* __AA_AUDIT_H */
diff --git a/security/security.c b/security/security.c
index 2c161101074d..5e9de8d0cdde 100644
--- a/security/security.c
+++ b/security/security.c
@@ -5450,7 +5450,20 @@ void security_audit_rule_free(void *lsmrule)
  */
 int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule)
 {
-	return call_int_hook(audit_rule_match, secid, field, op, lsmrule);
+	int rc;
+	bool match = false;
+	struct security_hook_list *hp;
+
+	hlist_for_each_entry(hp, &security_hook_heads.audit_rule_match, list) {
+		rc = hp->hook.audit_rule_match(secid, field, op, lsmrule,
+					       &match);
+		if (rc < 0)
+			return rc;
+		if (match)
+			break;
+	}
+
+	return match;
 }
 #endif /* CONFIG_AUDIT */
 
diff --git a/security/selinux/include/audit.h b/security/selinux/include/audit.h
index 29c7d4c86f6d..2d0799270426 100644
--- a/security/selinux/include/audit.h
+++ b/security/selinux/include/audit.h
@@ -45,11 +45,13 @@ void selinux_audit_rule_free(void *rule);
  *	@field: the field this rule refers to
  *	@op: the operator the rule uses
  *	@rule: pointer to the audit rule to check against
+ *	@match: if the context id matches the rule
  *
- *	Returns 1 if the context id matches the rule, 0 if it does not, and
- *	-errno on failure.
+ *	Returns 0 on success and -errno on failure. @match holds the match
+ *	result.
  */
-int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *rule);
+int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *rule,
+			     bool *match);
 
 /**
  *	selinux_audit_rule_known - check to see if rule contains selinux fields.
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index e33e55384b75..2946d28a25b1 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -3633,29 +3633,32 @@ int selinux_audit_rule_known(struct audit_krule *rule)
 	return 0;
 }
 
-int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
+int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule,
+			     bool *match)
 {
 	struct selinux_state *state = &selinux_state;
 	struct selinux_policy *policy;
 	struct context *ctxt;
 	struct mls_level *level;
 	struct selinux_audit_rule *rule = vrule;
-	int match = 0;
+	int rc = 0;
 
 	if (unlikely(!rule)) {
 		WARN_ONCE(1, "selinux_audit_rule_match: missing rule\n");
 		return -ENOENT;
 	}
 
-	if (!selinux_initialized())
+	if (!selinux_initialized()) {
+		*match = false;
 		return 0;
+	}
 
 	rcu_read_lock();
 
 	policy = rcu_dereference(state->policy);
 
 	if (rule->au_seqno < policy->latest_granting) {
-		match = -ESTALE;
+		rc = -ESTALE;
 		goto out;
 	}
 
@@ -3663,7 +3666,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	if (unlikely(!ctxt)) {
 		WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n",
 			  sid);
-		match = -ENOENT;
+		rc = -ENOENT;
 		goto out;
 	}
 
@@ -3674,10 +3677,10 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	case AUDIT_OBJ_USER:
 		switch (op) {
 		case Audit_equal:
-			match = (ctxt->user == rule->au_ctxt.user);
+			rc = (ctxt->user == rule->au_ctxt.user);
 			break;
 		case Audit_not_equal:
-			match = (ctxt->user != rule->au_ctxt.user);
+			rc = (ctxt->user != rule->au_ctxt.user);
 			break;
 		}
 		break;
@@ -3685,10 +3688,10 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	case AUDIT_OBJ_ROLE:
 		switch (op) {
 		case Audit_equal:
-			match = (ctxt->role == rule->au_ctxt.role);
+			rc = (ctxt->role == rule->au_ctxt.role);
 			break;
 		case Audit_not_equal:
-			match = (ctxt->role != rule->au_ctxt.role);
+			rc = (ctxt->role != rule->au_ctxt.role);
 			break;
 		}
 		break;
@@ -3696,10 +3699,10 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 	case AUDIT_OBJ_TYPE:
 		switch (op) {
 		case Audit_equal:
-			match = (ctxt->type == rule->au_ctxt.type);
+			rc = (ctxt->type == rule->au_ctxt.type);
 			break;
 		case Audit_not_equal:
-			match = (ctxt->type != rule->au_ctxt.type);
+			rc = (ctxt->type != rule->au_ctxt.type);
 			break;
 		}
 		break;
@@ -3712,39 +3715,42 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule)
 			 &ctxt->range.level[0] : &ctxt->range.level[1]);
 		switch (op) {
 		case Audit_equal:
-			match = mls_level_eq(&rule->au_ctxt.range.level[0],
-					     level);
+			rc = mls_level_eq(&rule->au_ctxt.range.level[0],
+					  level);
 			break;
 		case Audit_not_equal:
-			match = !mls_level_eq(&rule->au_ctxt.range.level[0],
-					      level);
+			rc = !mls_level_eq(&rule->au_ctxt.range.level[0],
+					   level);
 			break;
 		case Audit_lt:
-			match = (mls_level_dom(&rule->au_ctxt.range.level[0],
-					       level) &&
+			rc = (mls_level_dom(&rule->au_ctxt.range.level[0],
+					    level) &&
 				 !mls_level_eq(&rule->au_ctxt.range.level[0],
 					       level));
 			break;
 		case Audit_le:
-			match = mls_level_dom(&rule->au_ctxt.range.level[0],
-					      level);
+			rc = mls_level_dom(&rule->au_ctxt.range.level[0],
+					   level);
 			break;
 		case Audit_gt:
-			match = (mls_level_dom(level,
-					      &rule->au_ctxt.range.level[0]) &&
+			rc = (mls_level_dom(level,
+					    &rule->au_ctxt.range.level[0]) &&
 				 !mls_level_eq(level,
 					       &rule->au_ctxt.range.level[0]));
 			break;
 		case Audit_ge:
-			match = mls_level_dom(level,
-					      &rule->au_ctxt.range.level[0]);
+			rc = mls_level_dom(level,
+					   &rule->au_ctxt.range.level[0]);
 			break;
 		}
 	}
 
 out:
 	rcu_read_unlock();
-	return match;
+	if (rc < 0)
+		return rc;
+	*match = !!rc;
+	return 0;
 }
 
 static int aurule_avc_callback(u32 event)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 9a121ad53b16..ea0f0cf11ff3 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4764,11 +4764,15 @@ static int smack_audit_rule_known(struct audit_krule *krule)
  * @field: audit rule flags given from user-space
  * @op: required testing operator
  * @vrule: smack internal rule presentation
+ * @match: the match result
  *
  * The core Audit hook. It's used to take the decision of
  * whether to audit or not to audit a given object.
+ *
+ * Returns 0 on success or negative error code on failure.
  */
-static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule)
+static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
+				  bool *match)
 {
 	struct smack_known *skp;
 	char *rule = vrule;
@@ -4778,8 +4782,10 @@ static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule)
 		return -ENOENT;
 	}
 
-	if (field != AUDIT_SUBJ_USER && field != AUDIT_OBJ_USER)
+	if (field != AUDIT_SUBJ_USER && field != AUDIT_OBJ_USER) {
+		*match = false;
 		return 0;
+	}
 
 	skp = smack_from_secid(secid);
 
@@ -4789,10 +4795,11 @@ static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule)
 	 * label.
 	 */
 	if (op == Audit_equal)
-		return (rule == skp->smk_known);
-	if (op == Audit_not_equal)
-		return (rule != skp->smk_known);
-
+		*match = (rule == skp->smk_known);
+	else if (op == Audit_not_equal)
+		*match = (rule != skp->smk_known);
+	else
+		*match = false;
 	return 0;
 }
 
-- 
2.30.2

[PATCH bpf-next v4 11/20] bpf, lsm: Add disabled BPF LSM hook list

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

Add a disabled hooks list for BPF LSM. progs being attached to the
listed hooks will be rejected by the verifier.

Suggested-by: KP Singh <kpsingh@kernel.org>
Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 kernel/bpf/bpf_lsm.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index 08a338e1f231..e5feb6560fe6 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -36,6 +36,12 @@ BTF_SET_START(bpf_lsm_hooks)
 #undef LSM_HOOK
 BTF_SET_END(bpf_lsm_hooks)
 
+BTF_SET_START(bpf_lsm_disabled_hooks)
+BTF_ID(func, bpf_lsm_getprocattr)
+BTF_ID(func, bpf_lsm_setprocattr)
+BTF_ID(func, bpf_lsm_ismaclabel)
+BTF_SET_END(bpf_lsm_disabled_hooks)
+
 /* List of LSM hooks that should operate on 'current' cgroup regardless
  * of function signature.
  */
@@ -97,15 +103,24 @@ void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
 int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
 			const struct bpf_prog *prog)
 {
+	u32 btf_id = prog->aux->attach_btf_id;
+	const char *func_name = prog->aux->attach_func_name;
+
 	if (!prog->gpl_compatible) {
 		bpf_log(vlog,
 			"LSM programs must have a GPL compatible license\n");
 		return -EINVAL;
 	}
 
-	if (!btf_id_set_contains(&bpf_lsm_hooks, prog->aux->attach_btf_id)) {
+	if (btf_id_set_contains(&bpf_lsm_disabled_hooks, btf_id)) {
+		bpf_log(vlog, "attach_btf_id %u points to disabled hook %s\n",
+			btf_id, func_name);
+		return -EINVAL;
+	}
+
+	if (!btf_id_set_contains(&bpf_lsm_hooks, btf_id)) {
 		bpf_log(vlog, "attach_btf_id %u points to wrong type name %s\n",
-			prog->aux->attach_btf_id, prog->aux->attach_func_name);
+			btf_id, func_name);
 		return -EINVAL;
 	}
 
-- 
2.30.2

[PATCH bpf-next v4 12/20] bpf, lsm: Enable BPF LSM prog to read/write return value parameters

[Xu Kuohai]

From: Xu Kuohai <xukuohai@huawei.com>

Output parameters are used to refactor the LSM hook return values.
To make these hooks usable by bpf prog, it is necessary for bpf prog
to read and write these output return value parameters.

All return value parameters are added as the last parameter and are
always pointers to integer types. This patch uses BTF ID list to
specify which LMS hooks have return value parameters and enables bpf
prog to read/write the last parameters of these hooks in the verifier.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/bpf_lsm.h |  6 ++++++
 kernel/bpf/bpf_lsm.c    | 15 +++++++++++++++
 kernel/bpf/btf.c        | 16 +++++++++++++++-
 kernel/bpf/verifier.c   | 33 ++++++++++++++++++++++++++++++++-
 4 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
index 1de7ece5d36d..73e1f6dbec4a 100644
--- a/include/linux/bpf_lsm.h
+++ b/include/linux/bpf_lsm.h
@@ -45,6 +45,8 @@ void bpf_inode_storage_free(struct inode *inode);
 
 void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog, bpf_func_t *bpf_func);
 
+bool bpf_lsm_has_retval_param(const struct bpf_prog *prog);
+
 #else /* !CONFIG_BPF_LSM */
 
 static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
@@ -78,6 +80,10 @@ static inline void bpf_lsm_find_cgroup_shim(const struct bpf_prog *prog,
 {
 }
 
+static inline bool bpf_lsm_has_retval_param(const struct bpf_prog *prog)
+{
+	return false;
+}
 #endif /* CONFIG_BPF_LSM */
 
 #endif /* _LINUX_BPF_LSM_H */
diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index e5feb6560fe6..a8f8358c77e3 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -405,3 +405,18 @@ const struct bpf_verifier_ops lsm_verifier_ops = {
 	.get_func_proto = bpf_lsm_func_proto,
 	.is_valid_access = btf_ctx_access,
 };
+
+BTF_SET_START(retval_param_lsm_hooks)
+BTF_ID(func, bpf_lsm_inode_need_killpriv)
+BTF_ID(func, bpf_lsm_inode_getsecurity)
+BTF_ID(func, bpf_lsm_inode_listsecurity)
+BTF_ID(func, bpf_lsm_getselfattr)
+BTF_ID(func, bpf_lsm_key_getsecurity)
+BTF_ID(func, bpf_lsm_audit_rule_match)
+BTF_SET_END(retval_param_lsm_hooks)
+
+bool bpf_lsm_has_retval_param(const struct bpf_prog *prog)
+{
+	return btf_id_set_contains(&retval_param_lsm_hooks,
+				   prog->aux->attach_btf_id);
+}
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 4ff11779699e..df299d600b10 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6499,8 +6499,22 @@ bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 		 */
 		return true;
 
-	if (is_int_ptr(btf, t))
+
+	if (is_int_ptr(btf, t)) {
+		/* the retval param for LSM hook is always the last param. */
+		if (arg == nr_args - 1 &&
+		    prog->expected_attach_type == BPF_LSM_MAC &&
+		    bpf_lsm_has_retval_param(prog)) {
+			u32 id;
+
+			btf_type_skip_modifiers(btf, t->type, &id);
+			info->btf = btf;
+			/* the retval param should never be NULL */
+			info->reg_type = PTR_TO_BTF_ID | PTR_TRUSTED;
+			info->btf_id = id;
+		}
 		return true;
+	}
 
 	/* this is a pointer to another type */
 	for (i = 0; i < prog->aux->ctx_arg_info_size; i++) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index c0263fb5ca4b..a0bbef2d14e4 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6527,7 +6527,38 @@ static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
-	if (env->ops->btf_struct_access && !type_is_alloc(reg->type) && atype == BPF_WRITE) {
+	if (btf_type_is_int(t)) {
+		u32 tsize;
+		const char *tname;
+		const struct btf_type *err;
+		const char *access = atype == BPF_READ ? "read" : "write";
+
+		/* only BPF LSM is allowed */
+		if (WARN_ON_ONCE(env->prog->expected_attach_type != BPF_LSM_MAC)) {
+			verbose(env, "verifier internal error: not BPF LSM\n");
+			return -EACCES;
+		}
+
+		tname = btf_name_by_offset(reg->btf, t->name_off);
+		if (off != 0) {
+			verbose(env, "invalid %s offset: %d (expected 0, type=%s)\n",
+				access, off, tname);
+			return -EACCES;
+		}
+
+		err = btf_resolve_size(reg->btf, t, &tsize);
+		if (IS_ERR(err)) {
+			verbose(env, "unable to resolve the size of type '%s': %ld\n",
+				tname, PTR_ERR(err));
+			return PTR_ERR(err);
+		}
+		if (size != tsize) {
+			verbose(env, "invalid %s size: %d (expected %u, type=%s)\n",
+				access, size, tsize, tname);
+			return -EACCES;
+		}
+		ret = SCALAR_VALUE;
+	} else if (env->ops->btf_struct_access && !type_is_alloc(reg->type) && atype == BPF_WRITE) {
 		if (!btf_is_kernel(reg->btf)) {
 			verbose(env, "verifier internal error: reg->btf must be kernel btf\n");
 			return -EFAULT;
-- 
2.30.2

Re: [PATCH bpf-next v4 01/20] lsm: Refactor return value of LSM hook vm_enough_memory

[Serge Hallyn]

Jul 11, 2024 06:14:08 Xu Kuohai <xukuohai@huaweicloud.com>:

> From: Xu Kuohai <xukuohai@huawei.com>
>
> To be consistent with most LSM hooks, convert the return value of
> hook vm_enough_memory to 0 or a negative error code.
>
> Before:
> - Hook vm_enough_memory returns 1 if permission is granted, 0 if not.
> - LSM_RET_DEFAULT(vm_enough_memory_mm) is 1.
>
> After:
> - Hook vm_enough_memory reutrns 0 if permission is granted, negative
>   error code if not.
> - LSM_RET_DEFAULT(vm_enough_memory_mm) is 0.

I haven't done a detailed review yet, but based on the description this is definitely a good change.  Thank you.

> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
> include/linux/lsm_hook_defs.h |  2 +-
> include/linux/security.h      |  2 +-
> security/commoncap.c          | 11 +++--------
> security/security.c           | 11 +++++------
> security/selinux/hooks.c      | 15 ++++-----------
> 5 files changed, 14 insertions(+), 27 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 44488b1ab9a9..e6e6f8473955 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -48,7 +48,7 @@ LSM_HOOK(int, 0, quota_on, struct dentry *dentry)
> LSM_HOOK(int, 0, syslog, int type)
> LSM_HOOK(int, 0, settime, const struct timespec64 *ts,
>      const struct timezone *tz)
> -LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages)
> +LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages)
> LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm)
> LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct file *file)
> LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index de3af33e6ff5..454f96307cb9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -634,7 +634,7 @@ static inline int security_settime64(const struct timespec64 *ts,
>
> static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
> {
> -   return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages));
> +   return __vm_enough_memory(mm, pages, !cap_vm_enough_memory(mm, pages));
> }
>
> static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm)
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 162d96b3a676..cefad323a0b1 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1396,17 +1396,12 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
>   * Determine whether the allocation of a new virtual mapping by the current
>   * task is permitted.
>   *
> - * Return: 1 if permission is granted, 0 if not.
> + * Return: 0 if permission granted, negative error code if not.
>   */
> int cap_vm_enough_memory(struct mm_struct *mm, long pages)
> {
> -   int cap_sys_admin = 0;
> -
> -   if (cap_capable(current_cred(), &init_user_ns,
> -               CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) == 0)
> -       cap_sys_admin = 1;
> -
> -   return cap_sys_admin;
> +   return cap_capable(current_cred(), &init_user_ns, CAP_SYS_ADMIN,
> +              CAP_OPT_NOAUDIT);
> }
>
> /**
> diff --git a/security/security.c b/security/security.c
> index e5ca08789f74..3475f0cab3da 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1115,15 +1115,14 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
>     int rc;
>
>     /*
> -    * The module will respond with a positive value if
> -    * it thinks the __vm_enough_memory() call should be
> -    * made with the cap_sys_admin set. If all of the modules
> -    * agree that it should be set it will. If any module
> -    * thinks it should not be set it won't.
> +    * The module will respond with 0 if it thinks the __vm_enough_memory()
> +    * call should be made with the cap_sys_admin set. If all of the modules
> +    * agree that it should be set it will. If any module 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);
> -       if (rc <= 0) {
> +       if (rc < 0) {
>             cap_sys_admin = 0;
>             break;
>         }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7eed331e90f0..9cd5a8f1f6a3 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2202,23 +2202,16 @@ static int selinux_syslog(int type)
> }
>
> /*
> - * Check that a process has enough memory to allocate a new virtual
> - * mapping. 0 means there is enough memory for the allocation to
> - * succeed and -ENOMEM implies there is not.
> + * Check permission for allocating a new virtual mapping. Returns
> + * 0 if permission is granted, negative error code if not.
>   *
>   * Do not audit the selinux permission check, as this is applied to all
>   * processes that allocate mappings.
>   */
> static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
> {
> -   int rc, cap_sys_admin = 0;
> -
> -   rc = cred_has_capability(current_cred(), CAP_SYS_ADMIN,
> -                CAP_OPT_NOAUDIT, true);
> -   if (rc == 0)
> -       cap_sys_admin = 1;
> -
> -   return cap_sys_admin;
> +   return cred_has_capability(current_cred(), CAP_SYS_ADMIN,
> +                  CAP_OPT_NOAUDIT, true);
> }
>
> /* binprm security operations */
> --
> 2.30.2

Re: [PATCH bpf-next v4 02/20] lsm: Refactor return value of LSM hook inode_need_killpriv

[Serge Hallyn]

Jul 11, 2024 06:14:09 Xu Kuohai <xukuohai@huaweicloud.com>:

> From: Xu Kuohai <xukuohai@huawei.com>
>
> To be consistent with most LSM hooks, convert the return value of
> hook inode_need_killpriv to 0 or a negative error code.
>
> Before:
> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>   return > 0 if security_inode_killpriv is required, 0 if not, and < 0
>   to abort the operation.
>
> After:
> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>   return 0 on success and a negative error code on failure.
>   On success, hook inode_need_killpriv sets output param @need to true
>   if security_inode_killpriv is required, and false if not. When @need
>   is true, func security_inode_need_killpriv sets ATTR_KILL_PRIV flag
>   in @attr; when false, it clears the flag.
>   On failure, @need and @attr remains unchanged.
>
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>

It looks ok - though unnecessary (I'm assuming a later patch works better with this) - , but I'd be more comfortable if it was documented that any callers of the need_killpriv hook must set need to false before calling. Or if the hooks set need to false at start.


> ---
> fs/attr.c                     |  5 ++---
> fs/inode.c                    |  4 +---
> include/linux/lsm_hook_defs.h |  2 +-
> include/linux/security.h      | 20 ++++++++++++++++----
> security/commoncap.c          | 12 ++++++++----
> security/security.c           | 29 ++++++++++++++++++++++++-----
> 6 files changed, 52 insertions(+), 20 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 960a310581eb..aaadc721c982 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -427,11 +427,10 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
>         attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
>
>     if (ia_valid & ATTR_KILL_PRIV) {
> -       error = security_inode_need_killpriv(dentry);
> +       error = security_inode_need_killpriv(dentry, &ia_valid);
>         if (error < 0)
>             return error;
> -       if (error == 0)
> -           ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
> +       attr->ia_valid = ia_valid;
>     }
>
>     /*
> diff --git a/fs/inode.c b/fs/inode.c
> index 3a41f83a4ba5..cd335dc3a3bc 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2012,11 +2012,9 @@ int dentry_needs_remove_privs(struct mnt_idmap *idmap,
>         return 0;
>
>     mask = setattr_should_drop_suidgid(idmap, inode);
> -   ret = security_inode_need_killpriv(dentry);
> +   ret = security_inode_need_killpriv(dentry, &mask);
>     if (ret < 0)
>         return ret;
> -   if (ret)
> -       mask |= ATTR_KILL_PRIV;
>     return mask;
> }
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index e6e6f8473955..964849de424b 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -165,7 +165,7 @@ LSM_HOOK(int, 0, inode_remove_acl, struct mnt_idmap *idmap,
>      struct dentry *dentry, const char *acl_name)
> LSM_HOOK(void, LSM_RET_VOID, inode_post_remove_acl, struct mnt_idmap *idmap,
>      struct dentry *dentry, const char *acl_name)
> -LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry)
> +LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry, bool *need)
> LSM_HOOK(int, 0, inode_killpriv, struct mnt_idmap *idmap,
>      struct dentry *dentry)
> LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 454f96307cb9..1614ef5b2dd2 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -161,7 +161,7 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
>                const void *value, size_t size, int flags);
> int cap_inode_removexattr(struct mnt_idmap *idmap,
>               struct dentry *dentry, const char *name);
> -int cap_inode_need_killpriv(struct dentry *dentry);
> +int cap_inode_need_killpriv(struct dentry *dentry, bool *need);
> int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
> int cap_inode_getsecurity(struct mnt_idmap *idmap,
>               struct inode *inode, const char *name, void **buffer,
> @@ -389,7 +389,7 @@ int security_inode_listxattr(struct dentry *dentry);
> int security_inode_removexattr(struct mnt_idmap *idmap,
>                    struct dentry *dentry, const char *name);
> void security_inode_post_removexattr(struct dentry *dentry, const char *name);
> -int security_inode_need_killpriv(struct dentry *dentry);
> +int security_inode_need_killpriv(struct dentry *dentry, int *attr);
> int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
> int security_inode_getsecurity(struct mnt_idmap *idmap,
>                    struct inode *inode, const char *name,
> @@ -971,9 +971,21 @@ static inline void security_inode_post_removexattr(struct dentry *dentry,
>                            const char *name)
> { }
>
> -static inline int security_inode_need_killpriv(struct dentry *dentry)
> +static inline int security_inode_need_killpriv(struct dentry *dentry, int *attr)
> {
> -   return cap_inode_need_killpriv(dentry);
> +   int rc;
> +   bool need = false;
> +
> +   rc = cap_inode_need_killpriv(dentry, &need);
> +   if (rc < 0)
> +       return rc;
> +
> +   if (need)
> +       *attr |= ATTR_KILL_PRIV;
> +   else
> +       *attr &= ~ATTR_KILL_PRIV;
> +
> +   return 0;
> }
>
> static inline int security_inode_killpriv(struct mnt_idmap *idmap,
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cefad323a0b1..17d6188d22cf 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -286,21 +286,25 @@ int cap_capset(struct cred *new,
> /**
>   * cap_inode_need_killpriv - Determine if inode change affects privileges
>   * @dentry: The inode/dentry in being changed with change marked ATTR_KILL_PRIV
> + * @need: If inode_killpriv() is needed
>   *
>   * Determine if an inode having a change applied that's marked ATTR_KILL_PRIV
>   * affects the security markings on that inode, and if it is, should
>   * inode_killpriv() be invoked or the change rejected.
>   *
> - * Return: 1 if security.capability has a value, meaning inode_killpriv()
> - * is required, 0 otherwise, meaning inode_killpriv() is not required.
> + * Return: Always returns 0. If security.capability has a value, meaning
> + * inode_killpriv() is required, @need is set to true.
>   */
> -int cap_inode_need_killpriv(struct dentry *dentry)
> +int cap_inode_need_killpriv(struct dentry *dentry, bool *need)
> {
>     struct inode *inode = d_backing_inode(dentry);
>     int error;
>
>     error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0);
> -   return error > 0;
> +   if (error > 0)
> +       *need = true;
> +
> +   return 0;
> }
>
> /**
> diff --git a/security/security.c b/security/security.c
> index 3475f0cab3da..a4abcd86eb36 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2490,17 +2490,36 @@ void security_inode_post_removexattr(struct dentry *dentry, const char *name)
> /**
>   * security_inode_need_killpriv() - Check if security_inode_killpriv() required
>   * @dentry: associated dentry
> + * @attr: attribute flags
>   *
>   * Called when an inode has been changed to determine if
>   * security_inode_killpriv() should be called.
>   *
> - * Return: Return <0 on error to abort the inode change operation, return 0 if
> - *         security_inode_killpriv() does not need to be called, return >0 if
> - *         security_inode_killpriv() does need to be called.
> + * Return: Return 0 on success, negative error code on failure.
> + *         On success, set ATTR_KILL_PRIV flag in @attr when @need is true,
> + *         clears it when false.
>   */
> -int security_inode_need_killpriv(struct dentry *dentry)
> +int security_inode_need_killpriv(struct dentry *dentry, int *attr)
> {
> -   return call_int_hook(inode_need_killpriv, dentry);
> +   int rc;
> +   bool need = false;
> +   struct security_hook_list *hp;
> +
> +   hlist_for_each_entry(hp, &security_hook_heads.inode_need_killpriv,
> +                list) {
> +       rc = hp->hook.inode_need_killpriv(dentry, &need);
> +       if (rc < 0)
> +           return rc;
> +       if (need)
> +           break;
> +   }
> +
> +   if (need)
> +       *attr |= ATTR_KILL_PRIV;
> +   else
> +       *attr &= ~ATTR_KILL_PRIV;
> +
> +   return 0;
> }
>
> /**
> --
> 2.30.2

Re: [PATCH bpf-next v4 03/20] lsm: Refactor return value of LSM hook inode_getsecurity

[Simon Horman]

On Thu, Jul 11, 2024 at 07:18:51PM +0800, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> To be consistent with most LSM hooks, convert the return value of
> hook inode_getsecurity to 0 or a negative error code.
> 
> Before:
> - Hook inode_getsecurity returns size of buffer on success or a
>   negative error code on failure.
> 
> After:
> - Hook inode_getsecurity returns 0 on success or a negative error
>   code on failure. An output parameter @len is introduced to hold
>   the buffer size on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  fs/xattr.c                    | 19 ++++++++++---------
>  include/linux/lsm_hook_defs.h |  3 ++-
>  include/linux/security.h      | 12 ++++++------
>  security/commoncap.c          |  9 ++++++---
>  security/security.c           | 11 ++++++-----
>  security/selinux/hooks.c      | 16 ++++++----------
>  security/smack/smack_lsm.c    | 14 +++++++-------
>  7 files changed, 43 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index f8b643f91a98..f4e3bedf7272 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -339,27 +339,28 @@ xattr_getsecurity(struct mnt_idmap *idmap, struct inode *inode,
>  		  const char *name, void *value, size_t size)
>  {
>  	void *buffer = NULL;
> -	ssize_t len;
> +	int error;
> +	u32 len;
>  
>  	if (!value || !size) {
> -		len = security_inode_getsecurity(idmap, inode, name,
> -						 &buffer, false);
> +		error = security_inode_getsecurity(idmap, inode, name,
> +						   false, &buffer, &len);
>  		goto out_noalloc;
>  	}
>  
> -	len = security_inode_getsecurity(idmap, inode, name, &buffer,
> -					 true);
> -	if (len < 0)
> -		return len;
> +	error = security_inode_getsecurity(idmap, inode, name, true,
> +					   &buffer, &len);
> +	if (error)
> +		return error;
>  	if (size < len) {
> -		len = -ERANGE;
> +		error = -ERANGE;
>  		goto out;
>  	}
>  	memcpy(value, buffer, len);
>  out:
>  	kfree(buffer);
>  out_noalloc:
> -	return len;
> +	return error < 0 ? error : len;

Hi Xu Kuohai,

len is an unsigned 32-bit entity, but the return type of this function
is an unsigned value (ssize_t). So in theory, if len is very large,
a negative error value error will be returned.

>  }

Similarly for the handling of nattr in lsm_get_self_attr in
lsm_syscalls.c in a subsequent patch.

Flagged by Smatch.

...

Re: [PATCH bpf-next v4 00/20] Add return value range check for BPF LSM

[Paul Moore]

On Thu, Jul 11, 2024 at 7:13 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>
> From: Xu Kuohai <xukuohai@huawei.com>
>
> LSM BPF prog returning a positive number attached to the hook
> file_alloc_security makes kernel panic.

...

> Xu Kuohai (20):
>   lsm: Refactor return value of LSM hook vm_enough_memory
>   lsm: Refactor return value of LSM hook inode_need_killpriv
>   lsm: Refactor return value of LSM hook inode_getsecurity
>   lsm: Refactor return value of LSM hook inode_listsecurity
>   lsm: Refactor return value of LSM hook inode_copy_up_xattr
>   lsm: Refactor return value of LSM hook getselfattr
>   lsm: Refactor return value of LSM hook setprocattr
>   lsm: Refactor return value of LSM hook getprocattr
>   lsm: Refactor return value of LSM hook key_getsecurity
>   lsm: Refactor return value of LSM hook audit_rule_match
>   bpf, lsm: Add disabled BPF LSM hook list
>   bpf, lsm: Enable BPF LSM prog to read/write return value parameters
>   bpf, lsm: Add check for BPF LSM return value
>   bpf: Prevent tail call between progs attached to different hooks
>   bpf: Fix compare error in function retval_range_within
>   bpf: Add a special case for bitwise AND on range [-1, 0]
>   selftests/bpf: Avoid load failure for token_lsm.c
>   selftests/bpf: Add return value checks for failed tests
>   selftests/bpf: Add test for lsm tail call
>   selftests/bpf: Add verifier tests for bpf lsm

I'm not quite sure what happened, but it looks like patches 13/20
through 20/20 did not hit the mailing lists, see lore link below; did
you have any mail failures when sending the patchset?  Regardless, can
you sort this out and resend the patchset?

https://lore.kernel.org/all/20240711111908.3817636-1-xukuohai@huaweicloud.com

-- 
paul-moore.com

Re: [PATCH bpf-next v4 00/20] Add return value range check for BPF LSM

[Paul Moore]

On Fri, Jul 12, 2024 at 11:56 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Jul 11, 2024 at 7:13 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> >
> > From: Xu Kuohai <xukuohai@huawei.com>
> >
> > LSM BPF prog returning a positive number attached to the hook
> > file_alloc_security makes kernel panic.
>
> ...
>
> > Xu Kuohai (20):
> >   lsm: Refactor return value of LSM hook vm_enough_memory
> >   lsm: Refactor return value of LSM hook inode_need_killpriv
> >   lsm: Refactor return value of LSM hook inode_getsecurity
> >   lsm: Refactor return value of LSM hook inode_listsecurity
> >   lsm: Refactor return value of LSM hook inode_copy_up_xattr
> >   lsm: Refactor return value of LSM hook getselfattr
> >   lsm: Refactor return value of LSM hook setprocattr
> >   lsm: Refactor return value of LSM hook getprocattr
> >   lsm: Refactor return value of LSM hook key_getsecurity
> >   lsm: Refactor return value of LSM hook audit_rule_match
> >   bpf, lsm: Add disabled BPF LSM hook list
> >   bpf, lsm: Enable BPF LSM prog to read/write return value parameters
> >   bpf, lsm: Add check for BPF LSM return value
> >   bpf: Prevent tail call between progs attached to different hooks
> >   bpf: Fix compare error in function retval_range_within
> >   bpf: Add a special case for bitwise AND on range [-1, 0]
> >   selftests/bpf: Avoid load failure for token_lsm.c
> >   selftests/bpf: Add return value checks for failed tests
> >   selftests/bpf: Add test for lsm tail call
> >   selftests/bpf: Add verifier tests for bpf lsm
>
> I'm not quite sure what happened, but it looks like patches 13/20
> through 20/20 did not hit the mailing lists, see lore link below; did
> you have any mail failures when sending the patchset?  Regardless, can
> you sort this out and resend the patchset?
>
> https://lore.kernel.org/all/20240711111908.3817636-1-xukuohai@huaweicloud.com

Oh wait, it looks like the patchset was split in lore somehow,
nevermind.  The "missing" patches are here:

https://lore.kernel.org/all/20240711113828.3818398-1-xukuohai@huaweicloud.com

-- 
paul-moore.com

Re: [PATCH bpf-next v4 11/20] bpf, lsm: Add disabled BPF LSM hook list

[Alexei Starovoitov]

On Thu, Jul 11, 2024 at 07:18:59PM +0800, Xu Kuohai wrote:
> From: Xu Kuohai <xukuohai@huawei.com>
> 
> Add a disabled hooks list for BPF LSM. progs being attached to the
> listed hooks will be rejected by the verifier.
> 
> Suggested-by: KP Singh <kpsingh@kernel.org>
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>

Xu,

The patches 11 and higher are mostly independent from lsm refactoring.
Please send them as a separate patchset for bpf-next.
While lsm cleanups are being reviewed this lsm_disabled list can be
a bit larger temporarily.

Re: [PATCH bpf-next v4 00/20] Add return value range check for BPF LSM

[Paul Moore]

On Thu, Jul 11, 2024 at 7:13 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>
> From: Xu Kuohai <xukuohai@huawei.com>
>
> LSM BPF prog returning a positive number attached to the hook
> file_alloc_security makes kernel panic.
>
> Here is a panic log:
>
> [  441.235774] BUG: kernel NULL pointer dereference, address: 00000000000009
> [  441.236748] #PF: supervisor write access in kernel mode
> [  441.237429] #PF: error_code(0x0002) - not-present page
> [  441.238119] PGD 800000000b02f067 P4D 800000000b02f067 PUD b031067 PMD 0
> [  441.238990] Oops: 0002 [#1] PREEMPT SMP PTI
> [  441.239546] CPU: 0 PID: 347 Comm: loader Not tainted 6.8.0-rc6-gafe0cbf23373 #22
> [  441.240496] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b4
> [  441.241933] RIP: 0010:alloc_file+0x4b/0x190
> [  441.242485] Code: 8b 04 25 c0 3c 1f 00 48 8b b0 30 0c 00 00 e8 9c fe ff ff 48 3d 00 f0 ff fb
> [  441.244820] RSP: 0018:ffffc90000c67c40 EFLAGS: 00010203
> [  441.245484] RAX: ffff888006a891a0 RBX: ffffffff8223bd00 RCX: 0000000035b08000
> [  441.246391] RDX: ffff88800b95f7b0 RSI: 00000000001fc110 RDI: f089cd0b8088ffff
> [  441.247294] RBP: ffffc90000c67c58 R08: 0000000000000001 R09: 0000000000000001
> [  441.248209] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> [  441.249108] R13: ffffc90000c67c78 R14: ffffffff8223bd00 R15: fffffffffffffff4
> [  441.250007] FS:  00000000005f3300(0000) GS:ffff88803ec00000(0000) knlGS:0000000000000000
> [  441.251053] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  441.251788] CR2: 00000000000001a9 CR3: 000000000bdc4003 CR4: 0000000000170ef0
> [  441.252688] Call Trace:
> [  441.253011]  <TASK>
> [  441.253296]  ? __die+0x24/0x70
> [  441.253702]  ? page_fault_oops+0x15b/0x480
> [  441.254236]  ? fixup_exception+0x26/0x330
> [  441.254750]  ? exc_page_fault+0x6d/0x1c0
> [  441.255257]  ? asm_exc_page_fault+0x26/0x30
> [  441.255792]  ? alloc_file+0x4b/0x190
> [  441.256257]  alloc_file_pseudo+0x9f/0xf0
> [  441.256760]  __anon_inode_getfile+0x87/0x190
> [  441.257311]  ? lock_release+0x14e/0x3f0
> [  441.257808]  bpf_link_prime+0xe8/0x1d0
> [  441.258315]  bpf_tracing_prog_attach+0x311/0x570
> [  441.258916]  ? __pfx_bpf_lsm_file_alloc_security+0x10/0x10
> [  441.259605]  __sys_bpf+0x1bb7/0x2dc0
> [  441.260070]  __x64_sys_bpf+0x20/0x30
> [  441.260533]  do_syscall_64+0x72/0x140
> [  441.261004]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
> [  441.261643] RIP: 0033:0x4b0349
> [  441.262045] Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 88
> [  441.264355] RSP: 002b:00007fff74daee38 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
> [  441.265293] RAX: ffffffffffffffda RBX: 00007fff74daef30 RCX: 00000000004b0349
> [  441.266187] RDX: 0000000000000040 RSI: 00007fff74daee50 RDI: 000000000000001c
> [  441.267114] RBP: 000000000000001b R08: 00000000005ef820 R09: 0000000000000000
> [  441.268018] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
> [  441.268907] R13: 0000000000000004 R14: 00000000005ef018 R15: 00000000004004e8
>
> This is because the filesystem uses IS_ERR to check if the return value
> is an error code. If it is not, the filesystem takes the return value
> as a file pointer. Since the positive number returned by the BPF prog
> is not a real file pointer, this misinterpretation causes a panic.
>
> Since other LSM modules always return either a negative error code
> or a valid pointer, this specific issue only exists in BPF LSM. The
> proposed solution is to reject LSM BPF progs returning unexpected
> values in the verifier. This patch set adds return value check to
> ensure only BPF progs returning expected values are accepted.
>
> Since each LSM hook has different excepted return values, we need to
> know the expected return values for each individual hook to do the
> check. Earlier versions of the patch set used LSM hook annotations
> to specify the return value range for each hook. Based on Paul's
> suggestion, current version gets rid of such annotations and instead
> converts hook return values to a common pattern: return 0 on success
> and negative error code on failure.
>
> Basically, LSM hooks are divided into two types: hooks that return a
> negative error code and zero or other values, and hooks that do not
> return a negative error code. This patch set converts all hooks of the
> first type and part of the second type to return 0 on success and a
> negative error code on failure (see patches 1-10). For certain hooks,
> like ismaclabel and inode_xattr_skipcap, the hook name already imply
> that returning 0 or 1 is the best choice, so they are not converted.
> There are four unconverted hooks. Except for ismaclabel, which is not
> used by BPF LSM, the other three are specified with a BTF ID list to
> only return 0 or 1.

Thank you for following up on your initial work with this patchset, Xu
Kuohai.  It doesn't look like I'm going to be able to finish my review
by the end of the day today, so expect that a bit later, but so far I
think most of the changes look good and provide a nice improvement :)

-- 
paul-moore.com

Re: [PATCH bpf-next v4 02/20] lsm: Refactor return value of LSM hook inode_need_killpriv

[Xu Kuohai]

On 7/11/2024 10:15 PM, Serge Hallyn wrote:
> Jul 11, 2024 06:14:09 Xu Kuohai <xukuohai@huaweicloud.com>:
> 
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook inode_need_killpriv to 0 or a negative error code.
>>
>> Before:
>> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>>    return > 0 if security_inode_killpriv is required, 0 if not, and < 0
>>    to abort the operation.
>>
>> After:
>> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>>    return 0 on success and a negative error code on failure.
>>    On success, hook inode_need_killpriv sets output param @need to true
>>    if security_inode_killpriv is required, and false if not. When @need
>>    is true, func security_inode_need_killpriv sets ATTR_KILL_PRIV flag
>>    in @attr; when false, it clears the flag.
>>    On failure, @need and @attr remains unchanged.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> 
> It looks ok - though unnecessary (I'm assuming a later patch works better with this) - , but I'd be more comfortable if it was documented that any callers of the need_killpriv hook must set need to false before calling. Or if the hooks set need to false at start.
> 
>

I believe this is the only patch in the set that modifies 'inode_need_killpriv'.
I'll add an explanation for the initial value of '@need'.
Thanks.

Re: [PATCH bpf-next v4 03/20] lsm: Refactor return value of LSM hook inode_getsecurity

[Xu Kuohai]

On 7/12/2024 9:31 PM, Simon Horman wrote:
> On Thu, Jul 11, 2024 at 07:18:51PM +0800, Xu Kuohai wrote:
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook inode_getsecurity to 0 or a negative error code.
>>
>> Before:
>> - Hook inode_getsecurity returns size of buffer on success or a
>>    negative error code on failure.
>>
>> After:
>> - Hook inode_getsecurity returns 0 on success or a negative error
>>    code on failure. An output parameter @len is introduced to hold
>>    the buffer size on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   fs/xattr.c                    | 19 ++++++++++---------
>>   include/linux/lsm_hook_defs.h |  3 ++-
>>   include/linux/security.h      | 12 ++++++------
>>   security/commoncap.c          |  9 ++++++---
>>   security/security.c           | 11 ++++++-----
>>   security/selinux/hooks.c      | 16 ++++++----------
>>   security/smack/smack_lsm.c    | 14 +++++++-------
>>   7 files changed, 43 insertions(+), 41 deletions(-)
>>
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index f8b643f91a98..f4e3bedf7272 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -339,27 +339,28 @@ xattr_getsecurity(struct mnt_idmap *idmap, struct inode *inode,
>>   		  const char *name, void *value, size_t size)
>>   {
>>   	void *buffer = NULL;
>> -	ssize_t len;
>> +	int error;
>> +	u32 len;
>>   
>>   	if (!value || !size) {
>> -		len = security_inode_getsecurity(idmap, inode, name,
>> -						 &buffer, false);
>> +		error = security_inode_getsecurity(idmap, inode, name,
>> +						   false, &buffer, &len);
>>   		goto out_noalloc;
>>   	}
>>   
>> -	len = security_inode_getsecurity(idmap, inode, name, &buffer,
>> -					 true);
>> -	if (len < 0)
>> -		return len;
>> +	error = security_inode_getsecurity(idmap, inode, name, true,
>> +					   &buffer, &len);
>> +	if (error)
>> +		return error;
>>   	if (size < len) {
>> -		len = -ERANGE;
>> +		error = -ERANGE;
>>   		goto out;
>>   	}
>>   	memcpy(value, buffer, len);
>>   out:
>>   	kfree(buffer);
>>   out_noalloc:
>> -	return len;
>> +	return error < 0 ? error : len;
> 
> Hi Xu Kuohai,
> 
> len is an unsigned 32-bit entity, but the return type of this function
> is an unsigned value (ssize_t). So in theory, if len is very large,
> a negative error value error will be returned.
> 
>>   }
> 
> Similarly for the handling of nattr in lsm_get_self_attr in
> lsm_syscalls.c in a subsequent patch.
> 
> Flagged by Smatch.
> 
> ...
> 
>

OK, indeed there is no standardization that says ssize_t must be wider than u32.
I'll look into adding overflow checks. Thanks.

Re: [PATCH bpf-next v4 11/20] bpf, lsm: Add disabled BPF LSM hook list

[Xu Kuohai]

On 7/13/2024 1:56 AM, Alexei Starovoitov wrote:
> On Thu, Jul 11, 2024 at 07:18:59PM +0800, Xu Kuohai wrote:
>> From: Xu Kuohai <xukuohai@huawei.com>
>>
>> Add a disabled hooks list for BPF LSM. progs being attached to the
>> listed hooks will be rejected by the verifier.
>>
>> Suggested-by: KP Singh <kpsingh@kernel.org>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> 
> Xu,
> 
> The patches 11 and higher are mostly independent from lsm refactoring.
> Please send them as a separate patchset for bpf-next.
> While lsm cleanups are being reviewed this lsm_disabled list can be
> a bit larger temporarily.
> 

It's great to separate patches unrelated to bpf by temporarily extending
the lsm disabled list. I'll post an update. Thanks!

Re: [PATCH v4 1/20] lsm: Refactor return value of LSM hook vm_enough_memory

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook vm_enough_memory to 0 or a negative error code.
> 
> Before:
> - Hook vm_enough_memory returns 1 if permission is granted, 0 if not.
> - LSM_RET_DEFAULT(vm_enough_memory_mm) is 1.
> 
> After:
> - Hook vm_enough_memory reutrns 0 if permission is granted, negative
>   error code if not.
> - LSM_RET_DEFAULT(vm_enough_memory_mm) is 0.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      |  2 +-
>  security/commoncap.c          | 11 +++--------
>  security/security.c           | 11 +++++------
>  security/selinux/hooks.c      | 15 ++++-----------
>  5 files changed, 14 insertions(+), 27 deletions(-)

A nice improvement, thank you!

--
paul-moore.com

Re: [PATCH v4 2/20] lsm: Refactor return value of LSM hook inode_need_killpriv

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook inode_need_killpriv to 0 or a negative error code.
> 
> Before:
> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>   return > 0 if security_inode_killpriv is required, 0 if not, and < 0
>   to abort the operation.
> 
> After:
> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>   return 0 on success and a negative error code on failure.
>   On success, hook inode_need_killpriv sets output param @need to true
>   if security_inode_killpriv is required, and false if not. When @need
>   is true, func security_inode_need_killpriv sets ATTR_KILL_PRIV flag
>   in @attr; when false, it clears the flag.
>   On failure, @need and @attr remains unchanged.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  fs/attr.c                     |  5 ++---
>  fs/inode.c                    |  4 +---
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      | 20 ++++++++++++++++----
>  security/commoncap.c          | 12 ++++++++----
>  security/security.c           | 29 ++++++++++++++++++++++++-----
>  6 files changed, 52 insertions(+), 20 deletions(-)

In general I think a lot of these changes are a good improvement, thank
you very much for the time and effort you've spent on this.  However,
I'm not in favor of passing the new hook parameter as a way of reducing
the number of states represented by the security_inode_killpriv() return
value.  This particular hook may need to remain as one of the odd special
cases.

> diff --git a/fs/attr.c b/fs/attr.c
> index 960a310581eb..aaadc721c982 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -427,11 +427,10 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
>  		attr->ia_mtime = timestamp_truncate(attr->ia_mtime, inode);
>  
>  	if (ia_valid & ATTR_KILL_PRIV) {
> -		error = security_inode_need_killpriv(dentry);
> +		error = security_inode_need_killpriv(dentry, &ia_valid);
>  		if (error < 0)
>  			return error;
> -		if (error == 0)
> -			ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
> +		attr->ia_valid = ia_valid;
>  	}
>  
>  	/*
> diff --git a/fs/inode.c b/fs/inode.c
> index 3a41f83a4ba5..cd335dc3a3bc 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2012,11 +2012,9 @@ int dentry_needs_remove_privs(struct mnt_idmap *idmap,
>  		return 0;
>  
>  	mask = setattr_should_drop_suidgid(idmap, inode);
> -	ret = security_inode_need_killpriv(dentry);
> +	ret = security_inode_need_killpriv(dentry, &mask);
>  	if (ret < 0)
>  		return ret;
> -	if (ret)
> -		mask |= ATTR_KILL_PRIV;
>  	return mask;
>  }
>  
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index e6e6f8473955..964849de424b 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -165,7 +165,7 @@ LSM_HOOK(int, 0, inode_remove_acl, struct mnt_idmap *idmap,
>  	 struct dentry *dentry, const char *acl_name)
>  LSM_HOOK(void, LSM_RET_VOID, inode_post_remove_acl, struct mnt_idmap *idmap,
>  	 struct dentry *dentry, const char *acl_name)
> -LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry)
> +LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry, bool *need)
>  LSM_HOOK(int, 0, inode_killpriv, struct mnt_idmap *idmap,
>  	 struct dentry *dentry)
>  LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 454f96307cb9..1614ef5b2dd2 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -161,7 +161,7 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
>  		       const void *value, size_t size, int flags);
>  int cap_inode_removexattr(struct mnt_idmap *idmap,
>  			  struct dentry *dentry, const char *name);
> -int cap_inode_need_killpriv(struct dentry *dentry);
> +int cap_inode_need_killpriv(struct dentry *dentry, bool *need);
>  int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
>  int cap_inode_getsecurity(struct mnt_idmap *idmap,
>  			  struct inode *inode, const char *name, void **buffer,
> @@ -389,7 +389,7 @@ int security_inode_listxattr(struct dentry *dentry);
>  int security_inode_removexattr(struct mnt_idmap *idmap,
>  			       struct dentry *dentry, const char *name);
>  void security_inode_post_removexattr(struct dentry *dentry, const char *name);
> -int security_inode_need_killpriv(struct dentry *dentry);
> +int security_inode_need_killpriv(struct dentry *dentry, int *attr);
>  int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
>  int security_inode_getsecurity(struct mnt_idmap *idmap,
>  			       struct inode *inode, const char *name,
> @@ -971,9 +971,21 @@ static inline void security_inode_post_removexattr(struct dentry *dentry,
>  						   const char *name)
>  { }
>  
> -static inline int security_inode_need_killpriv(struct dentry *dentry)
> +static inline int security_inode_need_killpriv(struct dentry *dentry, int *attr)
>  {
> -	return cap_inode_need_killpriv(dentry);
> +	int rc;
> +	bool need = false;
> +
> +	rc = cap_inode_need_killpriv(dentry, &need);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (need)
> +		*attr |= ATTR_KILL_PRIV;
> +	else
> +		*attr &= ~ATTR_KILL_PRIV;
> +
> +	return 0;
>  }
>  
>  static inline int security_inode_killpriv(struct mnt_idmap *idmap,
> diff --git a/security/commoncap.c b/security/commoncap.c
> index cefad323a0b1..17d6188d22cf 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -286,21 +286,25 @@ int cap_capset(struct cred *new,
>  /**
>   * cap_inode_need_killpriv - Determine if inode change affects privileges
>   * @dentry: The inode/dentry in being changed with change marked ATTR_KILL_PRIV
> + * @need: If inode_killpriv() is needed
>   *
>   * Determine if an inode having a change applied that's marked ATTR_KILL_PRIV
>   * affects the security markings on that inode, and if it is, should
>   * inode_killpriv() be invoked or the change rejected.
>   *
> - * Return: 1 if security.capability has a value, meaning inode_killpriv()
> - * is required, 0 otherwise, meaning inode_killpriv() is not required.
> + * Return: Always returns 0. If security.capability has a value, meaning
> + * inode_killpriv() is required, @need is set to true.
>   */
> -int cap_inode_need_killpriv(struct dentry *dentry)
> +int cap_inode_need_killpriv(struct dentry *dentry, bool *need)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
>  	int error;
>  
>  	error = __vfs_getxattr(dentry, inode, XATTR_NAME_CAPS, NULL, 0);
> -	return error > 0;
> +	if (error > 0)
> +		*need = true;
> +
> +	return 0;
>  }
>  
>  /**
> diff --git a/security/security.c b/security/security.c
> index 3475f0cab3da..a4abcd86eb36 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2490,17 +2490,36 @@ void security_inode_post_removexattr(struct dentry *dentry, const char *name)
>  /**
>   * security_inode_need_killpriv() - Check if security_inode_killpriv() required
>   * @dentry: associated dentry
> + * @attr: attribute flags
>   *
>   * Called when an inode has been changed to determine if
>   * security_inode_killpriv() should be called.
>   *
> - * Return: Return <0 on error to abort the inode change operation, return 0 if
> - *         security_inode_killpriv() does not need to be called, return >0 if
> - *         security_inode_killpriv() does need to be called.
> + * Return: Return 0 on success, negative error code on failure.
> + *         On success, set ATTR_KILL_PRIV flag in @attr when @need is true,
> + *         clears it when false.
>   */
> -int security_inode_need_killpriv(struct dentry *dentry)
> +int security_inode_need_killpriv(struct dentry *dentry, int *attr)
>  {
> -	return call_int_hook(inode_need_killpriv, dentry);
> +	int rc;
> +	bool need = false;
> +	struct security_hook_list *hp;
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.inode_need_killpriv,
> +			     list) {
> +		rc = hp->hook.inode_need_killpriv(dentry, &need);
> +		if (rc < 0)
> +			return rc;
> +		if (need)
> +			break;
> +	}
> +
> +	if (need)
> +		*attr |= ATTR_KILL_PRIV;
> +	else
> +		*attr &= ~ATTR_KILL_PRIV;
> +
> +	return 0;
>  }
>  
>  /**
> -- 
> 2.30.2

--
paul-moore.com

Re: [PATCH v4 3/20] lsm: Refactor return value of LSM hook inode_getsecurity

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook inode_getsecurity to 0 or a negative error code.
> 
> Before:
> - Hook inode_getsecurity returns size of buffer on success or a
>   negative error code on failure.
> 
> After:
> - Hook inode_getsecurity returns 0 on success or a negative error
>   code on failure. An output parameter @len is introduced to hold
>   the buffer size on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  fs/xattr.c                    | 19 ++++++++++---------
>  include/linux/lsm_hook_defs.h |  3 ++-
>  include/linux/security.h      | 12 ++++++------
>  security/commoncap.c          |  9 ++++++---
>  security/security.c           | 11 ++++++-----
>  security/selinux/hooks.c      | 16 ++++++----------
>  security/smack/smack_lsm.c    | 14 +++++++-------
>  7 files changed, 43 insertions(+), 41 deletions(-)

Aside from Simon's concern over variable types, I saw a few other issues
when looking at this patch (below).

> diff --git a/security/commoncap.c b/security/commoncap.c
> index 17d6188d22cf..ff82e2ab6f8f 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -485,7 +485,10 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
>  	}
>  out_free:
>  	kfree(tmpbuf);
> -	return size;
> +	if (size < 0)
> +		return size;
> +	*len = size;
> +	return 0;
>  }

We should do a better job converting cap_inode_getsecurity(), create a
new local variable, e.g. 'int error', and use it to store and return the
error code instead of reusing @size.  I understand that what you've done
is easier, but I'd prefer to see it done properly.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9cd5a8f1f6a3..70792bba24d9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3407,7 +3407,7 @@ static int selinux_path_notify(const struct path *path, u64 mask,
>   */
>  static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>  				     struct inode *inode, const char *name,
> -				     void **buffer, bool alloc)
> +				     bool alloc, void **buffer, u32 *len)
>  {
>  	u32 size;
>  	int error;
> @@ -3440,14 +3440,14 @@ static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>  						&context, &size);
>  	if (error)
>  		return error;
> -	error = size;
> +	*len = size;

Depending on how you choose to resolve the variable type issue, you may
be able to pass @len directly to security_sid_to_context().

>  	if (alloc) {
>  		*buffer = context;
>  		goto out_nofree;
>  	}
>  	kfree(context);
>  out_nofree:
> -	return error;
> +	return 0;
>  }


--
paul-moore.com

Re: [PATCH v4 4/20] lsm: Refactor return value of LSM hook inode_listsecurity

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook inode_listsecurity to 0 or a negative error code.
> 
> Before:
> - Hook inode_listsecurity returns number of bytes used/required on
>   success or a negative error code on failure.
> 
> After:
> - Hook inode_listsecurity returns 0 on success or a negative error
>   code on failure. An output parameter @bytes is introduced to hold
>   the number of bytes used/required on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  fs/nfs/nfs4proc.c             |  5 ++++-
>  fs/xattr.c                    |  5 ++++-
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      |  7 ++++---
>  net/socket.c                  |  9 +++++----
>  security/security.c           | 29 +++++++++++++++++++++++++----
>  security/selinux/hooks.c      |  8 +++++---
>  security/smack/smack_lsm.c    |  6 ++++--
>  8 files changed, 52 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index a691fa10b3e9..6d75758ba3d5 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -7848,10 +7848,13 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
>  static ssize_t
>  nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len)
>  {
> +	size_t bytes;
>  	int len = 0;
>  
>  	if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) {
> -		len = security_inode_listsecurity(inode, list, list_len);
> +		len = security_inode_listsecurity(inode, list, list_len, &bytes);
> +		if (!len)
> +			len = bytes;
>  		if (len >= 0 && list_len && len > list_len)
>  			return -ERANGE;
>  	}

See my comments below.

> diff --git a/fs/xattr.c b/fs/xattr.c
> index f4e3bedf7272..ab7d7123a016 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -485,6 +485,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
>  {
>  	struct inode *inode = d_inode(dentry);
>  	ssize_t error;
> +	size_t bytes;
>  
>  	error = security_inode_listxattr(dentry);
>  	if (error)
> @@ -493,7 +494,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
>  	if (inode->i_op->listxattr) {
>  		error = inode->i_op->listxattr(dentry, list, size);
>  	} else {
> -		error = security_inode_listsecurity(inode, list, size);
> +		error = security_inode_listsecurity(inode, list, size, &bytes);
> +		if (!error)
> +			error = bytes;
>  		if (size && error > size)
>  			error = -ERANGE;

More on this below, but since the buffer length is fixed we are
already going to have to do a length comparison in the LSMs, why not
do the check and return -ERANGE there?

> diff --git a/net/socket.c b/net/socket.c
> index e416920e9399..43f0e3c9a6e0 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -571,12 +571,13 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
>  static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer,
>  				size_t size)
>  {
> -	ssize_t len;
> +	int err;
> +	size_t len;
>  	ssize_t used = 0;
>  
> -	len = security_inode_listsecurity(d_inode(dentry), buffer, size);
> -	if (len < 0)
> -		return len;
> +	err = security_inode_listsecurity(d_inode(dentry), buffer, size, &len);
> +	if (err < 0)
> +		return err;
>  	used += len;
>  	if (buffer) {
>  		if (size < used)

It doesn't show in the patch/diff, but if the LSM hook handles the length
comparison we can simplify the -ERANGE code in sockfs_listxattr().

> diff --git a/security/security.c b/security/security.c
> index 614f14cbfff7..26eea8f4cd74 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2597,20 +2597,41 @@ int security_inode_setsecurity(struct inode *inode, const char *name,
>   * @inode: inode
>   * @buffer: buffer
>   * @buffer_size: size of buffer
> + * @bytes: number of bytes used/required
>   *
>   * Copy the extended attribute names for the security labels associated with
>   * @inode into @buffer.  The maximum size of @buffer is specified by
>   * @buffer_size.  @buffer may be NULL to request the size of the buffer
>   * required.
>   *
> - * Return: Returns number of bytes used/required on success.
> + * Return: Returns 0 on success or a negative error code on failure.
>   */
>  int security_inode_listsecurity(struct inode *inode,
> -				char *buffer, size_t buffer_size)
> +				char *buffer, size_t buffer_size,
> +				size_t *bytes)
>  {
> +	int rc;
> +	size_t used;
> +	struct security_hook_list *hp;
> +
>  	if (unlikely(IS_PRIVATE(inode)))
> -		return 0;
> -	return call_int_hook(inode_listsecurity, inode, buffer, buffer_size);
> +		return *bytes = 0;
> +
> +	used = 0;
> +	hlist_for_each_entry(hp, &security_hook_heads.inode_listsecurity,
> +			     list) {
> +		rc = hp->hook.inode_listsecurity(inode, buffer, buffer_size,
> +						 &used);
> +		if (rc < 0)
> +			return rc;
> +		if (used != 0)
> +			break;
> +	}
> +
> +	*bytes = used;
> +
> +	return 0;
> +
>  }
>  EXPORT_SYMBOL(security_inode_listsecurity);

For reasons associated with the static_call work, we really need to
limit ourselves to the call_{int,void}_hook() macros on any new code.
The good news is that I think we can do that here as the existing
code isn't multi-LSM friendly.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 70792bba24d9..5dedd3917d57 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3481,16 +3481,18 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>  	return 0;
>  }
>  
> -static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
> +static int selinux_inode_listsecurity(struct inode *inode, char *buffer,
> +				      size_t buffer_size, size_t *bytes)
>  {
>  	const int len = sizeof(XATTR_NAME_SELINUX);
>  
>  	if (!selinux_initialized())
> -		return 0;
> +		return *bytes = 0;
>  
>  	if (buffer && len <= buffer_size)
>  		memcpy(buffer, XATTR_NAME_SELINUX, len);
> -	return len;
> +	*bytes = len;
> +	return 0;
>  }

Let's do something like below so we can catch -ERANGE in the LSMs
themselves.

  if (!selinux_initialized())
    return *bytes = 0;

  *bytes = sizeof(XATTR_NAME_SELINUX);
  if (len > buffer_size);
    return -ERANGE;
  if (buffer)
    memcpy(buffer, XATTR_NAME_SELINUX, *bytes);
  
  return 0;

>  static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index e7a5f6fd9a2d..6f73906bf7ea 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1611,16 +1611,18 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap,
>   * @inode: the object
>   * @buffer: where they go
>   * @buffer_size: size of buffer
> + * @bytes: number of data bytes in buffer
>   */
>  static int smack_inode_listsecurity(struct inode *inode, char *buffer,
> -				    size_t buffer_size)
> +				    size_t buffer_size, size_t *bytes)
>  {
>  	int len = sizeof(XATTR_NAME_SMACK);
>  
>  	if (buffer != NULL && len <= buffer_size)
>  		memcpy(buffer, XATTR_NAME_SMACK, len);
>  
> -	return len;
> +	*bytes = len;
> +	return 0;
>  }

A similar approach could be used here.

--
paul-moore.com

Re: [PATCH v4 5/20] lsm: Refactor return value of LSM hook inode_copy_up_xattr

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook inode_copy_up_xattr to 0 or a negative error code.
> 
> Before:
> - Hook inode_copy_up_xattr returns 0 when accepting xattr, 1 when
>   discarding xattr, -EOPNOTSUPP if it does not know xattr, or any
>   other negative error code otherwise.
> 
> After:
> - Hook inode_copy_up_xattr returns 0 when accepting xattr, *-ECANCELED*
>   when discarding xattr, -EOPNOTSUPP if it does not know xattr, or
>   any other negative error code otherwise.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  fs/overlayfs/copy_up.c            |  6 +++---
>  security/integrity/evm/evm_main.c |  2 +-
>  security/security.c               | 12 ++++++------
>  security/selinux/hooks.c          |  4 ++--
>  security/smack/smack_lsm.c        |  6 +++---
>  5 files changed, 15 insertions(+), 15 deletions(-)

...

> diff --git a/security/security.c b/security/security.c
> index 26eea8f4cd74..12215ca286af 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2675,18 +2675,18 @@ EXPORT_SYMBOL(security_inode_copy_up);
>   * lower layer to the union/overlay layer.   The caller is responsible for
>   * reading and writing the xattrs, this hook is merely a filter.
>   *
> - * Return: Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP
> - *         if the security module does not know about attribute, or a negative
> - *         error code to abort the copy up.
> + * Return: Returns 0 to accept the xattr, -ECANCELED to discard the xattr,
> + *         -EOPNOTSUPP if the security module does not know about attribute,
> + *         or a negative error code to abort the copy up.
>   */
>  int security_inode_copy_up_xattr(struct dentry *src, const char *name)
>  {
>  	int rc;
>  
>  	/*
> -	 * The implementation can return 0 (accept the xattr), 1 (discard the
> -	 * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
> -	 * any other error code in case of an error.
> +	 * The implementation can return 0 (accept the xattr), -ECANCELED
> +	 * (discard the xattr), -EOPNOTSUPP if it does not know anything
> +	 *  about the xattr or any other error code in case of an error.
>  	 */

Updating the comment here is good, but considering that we also discuss
the return value in the function header comment, I think it might be
better to just remove this comment entirely and leave the function header
comment as the single source.  Duplicated comments/docs tend to fall out
of sync and create confusion.

>  	rc = call_int_hook(inode_copy_up_xattr, src, name);
>  	if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))


--
paul-moore.com

Re: [PATCH v4 6/20] lsm: Refactor return value of LSM hook getselfattr

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook getselfattr to 0 or a negative error code.
> 
> Before:
> - Hook getselfattr returns number of attributes found on success
>   or a negative error code on failure.
> 
> After:
> - Hook getselfattr returns 0 on success or a negative error code
>   on failure. An output parameter @nattr is introduced to hold
>   the number of attributes found on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      |  5 +++--
>  security/apparmor/lsm.c       |  5 +++--
>  security/lsm_syscalls.c       |  6 +++++-
>  security/security.c           | 18 +++++++++++-------
>  security/selinux/hooks.c      | 13 +++++++++----
>  security/smack/smack_lsm.c    | 13 +++++++++----
>  7 files changed, 41 insertions(+), 21 deletions(-)

The getselfattr hook is different from the majority of the other LSM
hooks as getselfattr is used as part of lsm_get_self_attr(2) syscall and
not by other subsystems within the kernel.  Let's leave it as-is for now
as it is sufficiently special case that a deviation is okay.

--
paul-moore.com

Re: [PATCH v4 7/20] lsm: Refactor return value of LSM hook setprocattr

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook setprocattr to 0 or a negative error code.
> 
> Before:
> - Hook setprocattr returns the number of bytes written on success
>   or a negative error code on failure.
> 
> After:
> - Hook setprocattr returns 0 on success or a negative error code
>   on failure. An output parameter @wbytes is introduced to hold
>   the number of bytes written on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  fs/proc/base.c                |  5 +++--
>  include/linux/lsm_hook_defs.h |  3 ++-
>  include/linux/security.h      |  5 +++--
>  security/apparmor/lsm.c       | 10 +++++++---
>  security/security.c           |  8 +++++---
>  security/selinux/hooks.c      | 11 ++++++++---
>  security/smack/smack_lsm.c    | 14 ++++++++++----
>  7 files changed, 38 insertions(+), 18 deletions(-)

The security_setprocattr() hook is another odd case that we probably
just want to leave alone for two reasons:

1. With the move to LSM syscalls for getting/setting a task's LSM
attributes we are "freezing" the procfs API and not adding any new
entries to it.

2. The BPF LSM doesn't currently register any procfs entries.

I'd suggest leaving security_setprocattr() as-is and blocking it in
the BPF verifier, I can't see any reason why a BPF LSM would need
this hook.

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 72a1acd03675..9e1cf6cc674d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2740,6 +2740,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  {
>  	struct inode * inode = file_inode(file);
>  	struct task_struct *task;
> +	size_t wbytes;
>  	void *page;
>  	int rv;
>  
> @@ -2785,12 +2786,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>  
>  	rv = security_setprocattr(PROC_I(inode)->op.lsmid,
>  				  file->f_path.dentry->d_name.name, page,
> -				  count);
> +				  count, &wbytes);
>  	mutex_unlock(&current->signal->cred_guard_mutex);
>  out_free:
>  	kfree(page);
>  out:
> -	return rv;
> +	return rv < 0 ? rv : wbytes;
>  }
>  
>  static const struct file_operations proc_pid_attr_operations = {
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index dbc16f14f42f..2628514bb19c 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -287,7 +287,8 @@ LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
>  	 struct lsm_ctx *ctx, u32 size, u32 flags)
>  LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
>  	 char **value)
> -LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
> +LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size,
> +	 size_t *wbytes)
>  LSM_HOOK(int, 0, ismaclabel, const char *name)
>  LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata,
>  	 u32 *seclen)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 96a63e132abf..1f1a9696e65d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -496,7 +496,8 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
>  			 u32 size, u32 flags);
>  int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
>  			 char **value);
> -int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
> +int security_setprocattr(int lsmid, const char *name, void *value, size_t size,
> +			 size_t *wbytes);
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> @@ -1440,7 +1441,7 @@ static inline int security_getprocattr(struct task_struct *p, int lsmid,
>  }
>  
>  static inline int security_setprocattr(int lsmid, char *name, void *value,
> -				       size_t size)
> +				       size_t size, size_t *wbytes)
>  {
>  	return -EINVAL;
>  }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 72dd09993f28..6c8b1f8c5781 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -939,13 +939,17 @@ static int apparmor_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
>  }
>  
>  static int apparmor_setprocattr(const char *name, void *value,
> -				size_t size)
> +				size_t size, size_t *wbytes)
>  {
> +	int rc = -EINVAL;
>  	int attr = lsm_name_to_attr(name);
>  
>  	if (attr)
> -		return do_setattr(attr, value, size);
> -	return -EINVAL;
> +		rc = do_setattr(attr, value, size);
> +	if (rc < 0)
> +		return rc;
> +	*wbytes = rc;
> +	return 0;
>  }
>  
>  /**
> diff --git a/security/security.c b/security/security.c
> index 095e78efcb32..9685096dbf16 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -4141,20 +4141,22 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
>   * @name: attribute name
>   * @value: attribute value
>   * @size: attribute value size
> + * @wbytes: bytes written on success
>   *
>   * Write (set) the current task's attribute @name to @value, size @size if
>   * allowed.
>   *
> - * Return: Returns bytes written on success, a negative value otherwise.
> + * Return: Returns 0 on success, a negative error code otherwise.
>   */
> -int security_setprocattr(int lsmid, const char *name, void *value, size_t size)
> +int security_setprocattr(int lsmid, const char *name, void *value, size_t size,
> +			 size_t *wbytes)
>  {
>  	struct security_hook_list *hp;
>  
>  	hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
>  		if (lsmid != 0 && lsmid != hp->lsmid->id)
>  			continue;
> -		return hp->hook.setprocattr(name, value, size);
> +		return hp->hook.setprocattr(name, value, size, wbytes);
>  	}
>  	return LSM_RET_DEFAULT(setprocattr);
>  }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0d35bb93baca..7a73f3710025 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6589,13 +6589,18 @@ static int selinux_getprocattr(struct task_struct *p,
>  	return -EINVAL;
>  }
>  
> -static int selinux_setprocattr(const char *name, void *value, size_t size)
> +static int selinux_setprocattr(const char *name, void *value, size_t size,
> +			       size_t *wbytes)
>  {
> +	int rc = -EINVAL;
>  	int attr = lsm_name_to_attr(name);
>  
>  	if (attr)
> -		return selinux_lsm_setattr(attr, value, size);
> -	return -EINVAL;
> +		rc = selinux_lsm_setattr(attr, value, size);
> +	if (rc < 0)
> +		return rc;
> +	*wbytes = rc;
> +	return 0;
>  }
>  
>  static int selinux_ismaclabel(const char *name)
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 63d9c5f456c1..4265f2639106 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3797,19 +3797,25 @@ static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
>   * @name: the name of the attribute in /proc/.../attr
>   * @value: the value to set
>   * @size: the size of the value
> + * @wbytes: the length of the smack label written
>   *
>   * Sets the Smack value of the task. Only setting self
>   * is permitted and only with privilege
>   *
> - * Returns the length of the smack label or an error code
> + * Returns 0 on success or a negative error code
>   */
> -static int smack_setprocattr(const char *name, void *value, size_t size)
> +static int smack_setprocattr(const char *name, void *value, size_t size,
> +			     size_t *wbytes)
>  {
> +	int rc = -EINVAL;
>  	int attr = lsm_name_to_attr(name);
>  
>  	if (attr != LSM_ATTR_UNDEF)
> -		return do_setattr(attr, value, size);
> -	return -EINVAL;
> +		rc = do_setattr(attr, value, size);
> +	if (rc < 0)
> +		return rc;
> +	*wbytes = rc;
> +	return 0;
>  }
>  
>  /**
> -- 
> 2.30.2

--
paul-moore.com

Re: [PATCH v4 8/20] lsm: Refactor return value of LSM hook getprocattr

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook getprocattr to 0 or a negative error code.
> 
> Before:
> - Hook getprocattr returns length of value on success or a negative
>   error code on failure.
> 
> After:
> - Hook getprocattr returns 0 on success or a negative error code on
>   failure. An output parameter @len is introduced to hold the
>   length of value on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  fs/proc/base.c                |  5 ++++-
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      |  5 +++--
>  security/apparmor/lsm.c       |  7 +++++--
>  security/security.c           |  8 +++++---
>  security/selinux/hooks.c      | 16 +++++++++-------
>  security/smack/smack_lsm.c    | 11 ++++++-----
>  7 files changed, 33 insertions(+), 21 deletions(-)

The patch 07/20 comments also apply here.

--
paul-moore.com

Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook key_getsecurity

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook key_getsecurity to 0 or a negative error code.
> 
> Before:
> - Hook key_getsecurity returns length of value on success or a
>   negative error code on failure.
> 
> After:
> - Hook key_getsecurity returns 0 on success or a negative error
>   code on failure. An output parameter @len is introduced to hold
>   the length of value on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  include/linux/lsm_hook_defs.h |  3 ++-
>  include/linux/security.h      |  6 ++++--
>  security/keys/keyctl.c        | 11 ++++++++---
>  security/security.c           | 26 +++++++++++++++++++++-----
>  security/selinux/hooks.c      | 11 +++++------
>  security/smack/smack_lsm.c    | 21 +++++++++++----------
>  6 files changed, 51 insertions(+), 27 deletions(-)

...

> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index 4bc3e9398ee3..e9f857620f28 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1565,6 +1565,7 @@ long keyctl_get_security(key_serial_t keyid,
>  	struct key *key, *instkey;
>  	key_ref_t key_ref;
>  	char *context;
> +	size_t len;
>  	long ret;
>  
>  	key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
> @@ -1586,15 +1587,18 @@ long keyctl_get_security(key_serial_t keyid,
>  	}
>  
>  	key = key_ref_to_ptr(key_ref);
> -	ret = security_key_getsecurity(key, &context);
> -	if (ret == 0) {
> +	ret = security_key_getsecurity(key, &context, &len);
> +	if (ret < 0)
> +		goto error;

Since there is already an if-else pattern here you might as well stick
with that, for example:

  if (ret == 0) {
    ...
  } else if (ret > 0) {
    ...
  }

... you should probably add a comment that @ret is -ERRNO on failure,
but it doesn't look like you need an explicit test here since the error
case will normally fall through to the 'error' label (which you shouldn't
need anymore either).

> +	if (len == 0) {
>  		/* if no information was returned, give userspace an empty
>  		 * string */
>  		ret = 1;
>  		if (buffer && buflen > 0 &&
>  		    copy_to_user(buffer, "", 1) != 0)
>  			ret = -EFAULT;
> -	} else if (ret > 0) {
> +	} else {
> +		ret = len;
>  		/* return as much data as there's room for */
>  		if (buffer && buflen > 0) {
>  			if (buflen > ret)
> @@ -1607,6 +1611,7 @@ long keyctl_get_security(key_serial_t keyid,
>  		kfree(context);
>  	}
>  
> +error:
>  	key_ref_put(key_ref);
>  	return ret;
>  }
> diff --git a/security/security.c b/security/security.c
> index 9dd2ae6cf763..2c161101074d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
>   * security_key_getsecurity() - Get the key's security label
>   * @key: key
>   * @buffer: security label buffer
> + * @len: the length of @buffer (including terminating NULL) on success
>   *
>   * Get a textual representation of the security context attached to a key for
>   * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
>   * storage for the NUL-terminated string and the caller should free it.
>   *
> - * Return: Returns the length of @buffer (including terminating NUL) or -ve if
> - *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
> - *         there is no security label assigned to the key.
> + * Return: Returns 0 on success or -ve if an error occurs. May also return 0
> + *         (and a NULL buffer pointer) if there is no security label assigned
> + *         to the key.
>   */
> -int security_key_getsecurity(struct key *key, char **buffer)
> +int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
>  {
> +	int rc;
> +	size_t n = 0;
> +	struct security_hook_list *hp;
> +
>  	*buffer = NULL;
> -	return call_int_hook(key_getsecurity, key, buffer);
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
> +		rc = hp->hook.key_getsecurity(key, buffer, &n);
> +		if (rc < 0)
> +			return rc;
> +		if (n)
> +			break;
> +	}
> +
> +	*len = n;
> +
> +	return 0;
>  }

Help me understand why we can't continue to use the call_int_hook()
macro here?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 16cd336aab3d..747ec602dec0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6737,18 +6737,17 @@ static int selinux_key_permission(key_ref_t key_ref,
>  	return avc_has_perm(sid, ksec->sid, SECCLASS_KEY, perm, NULL);
>  }
>  
> -static int selinux_key_getsecurity(struct key *key, char **_buffer)
> +static int selinux_key_getsecurity(struct key *key, char **_buffer,
> +				   size_t *_len)
>  {
>  	struct key_security_struct *ksec = key->security;
>  	char *context = NULL;
> -	unsigned len;
> +	u32 context_len;

Since @len doesn't collide with the parameter, you might as well just
stick with @len as the local variable name.

>  	int rc;
>  
> -	rc = security_sid_to_context(ksec->sid,
> -				     &context, &len);
> -	if (!rc)
> -		rc = len;
> +	rc = security_sid_to_context(ksec->sid, &context, &context_len);
>  	*_buffer = context;
> +	*_len = context_len;
>  	return rc;
>  }

--
paul-moore.com

Re: [PATCH v4 10/20] lsm: Refactor return value of LSM hook audit_rule_match

[Paul Moore]

On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook audit_rule_match to 0 or a negative error code.
> 
> Before:
> - Hook audit_rule_match returns 1 if the rule matches, 0 if it not,
>   and negative error code otherwise.
> 
> After:
> - Hook audit_rule_match returns 0 on success or a negative error
>   code on failure. An output parameter @match is introduced to hold
>   the match result on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  include/linux/lsm_hook_defs.h     |  3 +-
>  security/apparmor/audit.c         | 22 ++++++-------
>  security/apparmor/include/audit.h |  2 +-
>  security/security.c               | 15 ++++++++-
>  security/selinux/include/audit.h  |  8 +++--
>  security/selinux/ss/services.c    | 54 +++++++++++++++++--------------
>  security/smack/smack_lsm.c        | 19 +++++++----
>  7 files changed, 75 insertions(+), 48 deletions(-)

This is another odd hook, and similar to some of the others in this
patchset, I'm not sure how applicable this would be to a BPF-based
LSM.  I suspect you could safely block this from a BPF LSM and no one
would notice or be upset.

However, if we did want to keep this hook for a BPF LSM, I think it
might be better to encode the "match" results in the return value, just
sticking with a more conventional 0/errno approach.  What do you think
about 0:found/ok, -ENOENT:missing/ok, -ERRNO:other/error?  Yes, some
of the existing LSM audit_match code uses -ENOENT but looking quickly
at those error conditions it seems that we could consider them
equivalent to a "missing" or "failed match" result and use -ENOENT for
both.  If you're really not happy with that overloading, we could use
something like -ENOMSG:missing/ok instead.

Thoughts?

--
paul-moore.com

Re: [PATCH bpf-next v4 00/20] Add return value range check for BPF LSM

[Paul Moore]

On Fri, Jul 12, 2024 at 5:44 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Jul 11, 2024 at 7:13 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> > From: Xu Kuohai <xukuohai@huawei.com>
> >
> > LSM BPF prog returning a positive number attached to the hook
> > file_alloc_security makes kernel panic.
> >
> > Here is a panic log:
> >
> > [  441.235774] BUG: kernel NULL pointer dereference, address: 00000000000009
> > [  441.236748] #PF: supervisor write access in kernel mode
> > [  441.237429] #PF: error_code(0x0002) - not-present page
> > [  441.238119] PGD 800000000b02f067 P4D 800000000b02f067 PUD b031067 PMD 0
> > [  441.238990] Oops: 0002 [#1] PREEMPT SMP PTI
> > [  441.239546] CPU: 0 PID: 347 Comm: loader Not tainted 6.8.0-rc6-gafe0cbf23373 #22
> > [  441.240496] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b4
> > [  441.241933] RIP: 0010:alloc_file+0x4b/0x190
> > [  441.242485] Code: 8b 04 25 c0 3c 1f 00 48 8b b0 30 0c 00 00 e8 9c fe ff ff 48 3d 00 f0 ff fb
> > [  441.244820] RSP: 0018:ffffc90000c67c40 EFLAGS: 00010203
> > [  441.245484] RAX: ffff888006a891a0 RBX: ffffffff8223bd00 RCX: 0000000035b08000
> > [  441.246391] RDX: ffff88800b95f7b0 RSI: 00000000001fc110 RDI: f089cd0b8088ffff
> > [  441.247294] RBP: ffffc90000c67c58 R08: 0000000000000001 R09: 0000000000000001
> > [  441.248209] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> > [  441.249108] R13: ffffc90000c67c78 R14: ffffffff8223bd00 R15: fffffffffffffff4
> > [  441.250007] FS:  00000000005f3300(0000) GS:ffff88803ec00000(0000) knlGS:0000000000000000
> > [  441.251053] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [  441.251788] CR2: 00000000000001a9 CR3: 000000000bdc4003 CR4: 0000000000170ef0
> > [  441.252688] Call Trace:
> > [  441.253011]  <TASK>
> > [  441.253296]  ? __die+0x24/0x70
> > [  441.253702]  ? page_fault_oops+0x15b/0x480
> > [  441.254236]  ? fixup_exception+0x26/0x330
> > [  441.254750]  ? exc_page_fault+0x6d/0x1c0
> > [  441.255257]  ? asm_exc_page_fault+0x26/0x30
> > [  441.255792]  ? alloc_file+0x4b/0x190
> > [  441.256257]  alloc_file_pseudo+0x9f/0xf0
> > [  441.256760]  __anon_inode_getfile+0x87/0x190
> > [  441.257311]  ? lock_release+0x14e/0x3f0
> > [  441.257808]  bpf_link_prime+0xe8/0x1d0
> > [  441.258315]  bpf_tracing_prog_attach+0x311/0x570
> > [  441.258916]  ? __pfx_bpf_lsm_file_alloc_security+0x10/0x10
> > [  441.259605]  __sys_bpf+0x1bb7/0x2dc0
> > [  441.260070]  __x64_sys_bpf+0x20/0x30
> > [  441.260533]  do_syscall_64+0x72/0x140
> > [  441.261004]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
> > [  441.261643] RIP: 0033:0x4b0349
> > [  441.262045] Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 88
> > [  441.264355] RSP: 002b:00007fff74daee38 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
> > [  441.265293] RAX: ffffffffffffffda RBX: 00007fff74daef30 RCX: 00000000004b0349
> > [  441.266187] RDX: 0000000000000040 RSI: 00007fff74daee50 RDI: 000000000000001c
> > [  441.267114] RBP: 000000000000001b R08: 00000000005ef820 R09: 0000000000000000
> > [  441.268018] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
> > [  441.268907] R13: 0000000000000004 R14: 00000000005ef018 R15: 00000000004004e8
> >
> > This is because the filesystem uses IS_ERR to check if the return value
> > is an error code. If it is not, the filesystem takes the return value
> > as a file pointer. Since the positive number returned by the BPF prog
> > is not a real file pointer, this misinterpretation causes a panic.
> >
> > Since other LSM modules always return either a negative error code
> > or a valid pointer, this specific issue only exists in BPF LSM. The
> > proposed solution is to reject LSM BPF progs returning unexpected
> > values in the verifier. This patch set adds return value check to
> > ensure only BPF progs returning expected values are accepted.
> >
> > Since each LSM hook has different excepted return values, we need to
> > know the expected return values for each individual hook to do the
> > check. Earlier versions of the patch set used LSM hook annotations
> > to specify the return value range for each hook. Based on Paul's
> > suggestion, current version gets rid of such annotations and instead
> > converts hook return values to a common pattern: return 0 on success
> > and negative error code on failure.
> >
> > Basically, LSM hooks are divided into two types: hooks that return a
> > negative error code and zero or other values, and hooks that do not
> > return a negative error code. This patch set converts all hooks of the
> > first type and part of the second type to return 0 on success and a
> > negative error code on failure (see patches 1-10). For certain hooks,
> > like ismaclabel and inode_xattr_skipcap, the hook name already imply
> > that returning 0 or 1 is the best choice, so they are not converted.
> > There are four unconverted hooks. Except for ismaclabel, which is not
> > used by BPF LSM, the other three are specified with a BTF ID list to
> > only return 0 or 1.
>
> Thank you for following up on your initial work with this patchset, Xu
> Kuohai.  It doesn't look like I'm going to be able to finish my review
> by the end of the day today, so expect that a bit later, but so far I
> think most of the changes look good and provide a nice improvement :)

You should have my feedback now, let me know if you have any questions.

One additional comment I might make is that you may either want to
wait until after v6.11-rc1 is released and I've had a chance to rebase
the lsm/{dev,next} branches and merge the patchsets which are
currently queued; there are a few patches queued up which will have an
impact on this work.  While it's an unstable branch, you can take a
peek at those queues patches in the lsm/dev-staging branch.

https://github.com/LinuxSecurityModule/kernel/blob/main/README.md

-- 
paul-moore.com

Re: [PATCH bpf-next v4 00/20] Add return value range check for BPF LSM

[Xu Kuohai]

On 7/19/2024 10:13 AM, Paul Moore wrote:
> On Fri, Jul 12, 2024 at 5:44 PM Paul Moore <paul@paul-moore.com> wrote:
>> On Thu, Jul 11, 2024 at 7:13 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>> From: Xu Kuohai <xukuohai@huawei.com>
>>>
>>> LSM BPF prog returning a positive number attached to the hook
>>> file_alloc_security makes kernel panic.
>>>
>>> Here is a panic log:
>>>
>>> [  441.235774] BUG: kernel NULL pointer dereference, address: 00000000000009
>>> [  441.236748] #PF: supervisor write access in kernel mode
>>> [  441.237429] #PF: error_code(0x0002) - not-present page
>>> [  441.238119] PGD 800000000b02f067 P4D 800000000b02f067 PUD b031067 PMD 0
>>> [  441.238990] Oops: 0002 [#1] PREEMPT SMP PTI
>>> [  441.239546] CPU: 0 PID: 347 Comm: loader Not tainted 6.8.0-rc6-gafe0cbf23373 #22
>>> [  441.240496] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.15.0-0-g2dd4b4
>>> [  441.241933] RIP: 0010:alloc_file+0x4b/0x190
>>> [  441.242485] Code: 8b 04 25 c0 3c 1f 00 48 8b b0 30 0c 00 00 e8 9c fe ff ff 48 3d 00 f0 ff fb
>>> [  441.244820] RSP: 0018:ffffc90000c67c40 EFLAGS: 00010203
>>> [  441.245484] RAX: ffff888006a891a0 RBX: ffffffff8223bd00 RCX: 0000000035b08000
>>> [  441.246391] RDX: ffff88800b95f7b0 RSI: 00000000001fc110 RDI: f089cd0b8088ffff
>>> [  441.247294] RBP: ffffc90000c67c58 R08: 0000000000000001 R09: 0000000000000001
>>> [  441.248209] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
>>> [  441.249108] R13: ffffc90000c67c78 R14: ffffffff8223bd00 R15: fffffffffffffff4
>>> [  441.250007] FS:  00000000005f3300(0000) GS:ffff88803ec00000(0000) knlGS:0000000000000000
>>> [  441.251053] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> [  441.251788] CR2: 00000000000001a9 CR3: 000000000bdc4003 CR4: 0000000000170ef0
>>> [  441.252688] Call Trace:
>>> [  441.253011]  <TASK>
>>> [  441.253296]  ? __die+0x24/0x70
>>> [  441.253702]  ? page_fault_oops+0x15b/0x480
>>> [  441.254236]  ? fixup_exception+0x26/0x330
>>> [  441.254750]  ? exc_page_fault+0x6d/0x1c0
>>> [  441.255257]  ? asm_exc_page_fault+0x26/0x30
>>> [  441.255792]  ? alloc_file+0x4b/0x190
>>> [  441.256257]  alloc_file_pseudo+0x9f/0xf0
>>> [  441.256760]  __anon_inode_getfile+0x87/0x190
>>> [  441.257311]  ? lock_release+0x14e/0x3f0
>>> [  441.257808]  bpf_link_prime+0xe8/0x1d0
>>> [  441.258315]  bpf_tracing_prog_attach+0x311/0x570
>>> [  441.258916]  ? __pfx_bpf_lsm_file_alloc_security+0x10/0x10
>>> [  441.259605]  __sys_bpf+0x1bb7/0x2dc0
>>> [  441.260070]  __x64_sys_bpf+0x20/0x30
>>> [  441.260533]  do_syscall_64+0x72/0x140
>>> [  441.261004]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
>>> [  441.261643] RIP: 0033:0x4b0349
>>> [  441.262045] Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 88
>>> [  441.264355] RSP: 002b:00007fff74daee38 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
>>> [  441.265293] RAX: ffffffffffffffda RBX: 00007fff74daef30 RCX: 00000000004b0349
>>> [  441.266187] RDX: 0000000000000040 RSI: 00007fff74daee50 RDI: 000000000000001c
>>> [  441.267114] RBP: 000000000000001b R08: 00000000005ef820 R09: 0000000000000000
>>> [  441.268018] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000004
>>> [  441.268907] R13: 0000000000000004 R14: 00000000005ef018 R15: 00000000004004e8
>>>
>>> This is because the filesystem uses IS_ERR to check if the return value
>>> is an error code. If it is not, the filesystem takes the return value
>>> as a file pointer. Since the positive number returned by the BPF prog
>>> is not a real file pointer, this misinterpretation causes a panic.
>>>
>>> Since other LSM modules always return either a negative error code
>>> or a valid pointer, this specific issue only exists in BPF LSM. The
>>> proposed solution is to reject LSM BPF progs returning unexpected
>>> values in the verifier. This patch set adds return value check to
>>> ensure only BPF progs returning expected values are accepted.
>>>
>>> Since each LSM hook has different excepted return values, we need to
>>> know the expected return values for each individual hook to do the
>>> check. Earlier versions of the patch set used LSM hook annotations
>>> to specify the return value range for each hook. Based on Paul's
>>> suggestion, current version gets rid of such annotations and instead
>>> converts hook return values to a common pattern: return 0 on success
>>> and negative error code on failure.
>>>
>>> Basically, LSM hooks are divided into two types: hooks that return a
>>> negative error code and zero or other values, and hooks that do not
>>> return a negative error code. This patch set converts all hooks of the
>>> first type and part of the second type to return 0 on success and a
>>> negative error code on failure (see patches 1-10). For certain hooks,
>>> like ismaclabel and inode_xattr_skipcap, the hook name already imply
>>> that returning 0 or 1 is the best choice, so they are not converted.
>>> There are four unconverted hooks. Except for ismaclabel, which is not
>>> used by BPF LSM, the other three are specified with a BTF ID list to
>>> only return 0 or 1.
>>
>> Thank you for following up on your initial work with this patchset, Xu
>> Kuohai.  It doesn't look like I'm going to be able to finish my review
>> by the end of the day today, so expect that a bit later, but so far I
>> think most of the changes look good and provide a nice improvement :)
> 
> You should have my feedback now, let me know if you have any questions.
> 
> One additional comment I might make is that you may either want to
> wait until after v6.11-rc1 is released and I've had a chance to rebase
> the lsm/{dev,next} branches and merge the patchsets which are
> currently queued; there are a few patches queued up which will have an
> impact on this work.  While it's an unstable branch, you can take a
> peek at those queues patches in the lsm/dev-staging branch.
> 
> https://github.com/LinuxSecurityModule/kernel/blob/main/README.md
> 

Got it, thanks for your valuable time and feedback! The individual
comment will be replied once I'm sure I understand it or confirmed
the next step.

Additionally, for the next update, I'll split the series into two,
as the refactoring patches and the BPF patches are not closely
related.

Re: [PATCH v4 2/20] lsm: Refactor return value of LSM hook inode_need_killpriv

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook inode_need_killpriv to 0 or a negative error code.
>>
>> Before:
>> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>>    return > 0 if security_inode_killpriv is required, 0 if not, and < 0
>>    to abort the operation.
>>
>> After:
>> - Both hook inode_need_killpriv and func security_inode_need_killpriv
>>    return 0 on success and a negative error code on failure.
>>    On success, hook inode_need_killpriv sets output param @need to true
>>    if security_inode_killpriv is required, and false if not. When @need
>>    is true, func security_inode_need_killpriv sets ATTR_KILL_PRIV flag
>>    in @attr; when false, it clears the flag.
>>    On failure, @need and @attr remains unchanged.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   fs/attr.c                     |  5 ++---
>>   fs/inode.c                    |  4 +---
>>   include/linux/lsm_hook_defs.h |  2 +-
>>   include/linux/security.h      | 20 ++++++++++++++++----
>>   security/commoncap.c          | 12 ++++++++----
>>   security/security.c           | 29 ++++++++++++++++++++++++-----
>>   6 files changed, 52 insertions(+), 20 deletions(-)
> 
> In general I think a lot of these changes are a good improvement, thank
> you very much for the time and effort you've spent on this.  However,
> I'm not in favor of passing the new hook parameter as a way of reducing
> the number of states represented by the security_inode_killpriv() return
> value.  This particular hook may need to remain as one of the odd special
> cases.
> 

I learned from previous discussions [1] to use a new output parameter to store
odd return values. Actually, I am not in favor of this method either, especially
since it requires extra work to enable BPF to access the output parameter. I
think we could just keep it as-is.

[1] https://lore.kernel.org/bpf/CAHC9VhQ_sTmoXwQ_AVfjTYQe4KR-uTnksPVfsei5JZ+VDJBQkA@mail.gmail.com/

Re: [PATCH v4 3/20] lsm: Refactor return value of LSM hook inode_getsecurity

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook inode_getsecurity to 0 or a negative error code.
>>
>> Before:
>> - Hook inode_getsecurity returns size of buffer on success or a
>>    negative error code on failure.
>>
>> After:
>> - Hook inode_getsecurity returns 0 on success or a negative error
>>    code on failure. An output parameter @len is introduced to hold
>>    the buffer size on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   fs/xattr.c                    | 19 ++++++++++---------
>>   include/linux/lsm_hook_defs.h |  3 ++-
>>   include/linux/security.h      | 12 ++++++------
>>   security/commoncap.c          |  9 ++++++---
>>   security/security.c           | 11 ++++++-----
>>   security/selinux/hooks.c      | 16 ++++++----------
>>   security/smack/smack_lsm.c    | 14 +++++++-------
>>   7 files changed, 43 insertions(+), 41 deletions(-)
> 
> Aside from Simon's concern over variable types, I saw a few other issues
> when looking at this patch (below).
> 
>> diff --git a/security/commoncap.c b/security/commoncap.c
>> index 17d6188d22cf..ff82e2ab6f8f 100644
>> --- a/security/commoncap.c
>> +++ b/security/commoncap.c
>> @@ -485,7 +485,10 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
>>   	}
>>   out_free:
>>   	kfree(tmpbuf);
>> -	return size;
>> +	if (size < 0)
>> +		return size;
>> +	*len = size;
>> +	return 0;
>>   }
> 
> We should do a better job converting cap_inode_getsecurity(), create a
> new local variable, e.g. 'int error', and use it to store and return the
> error code instead of reusing @size.  I understand that what you've done
> is easier, but I'd prefer to see it done properly.
>

Got it

>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 9cd5a8f1f6a3..70792bba24d9 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3407,7 +3407,7 @@ static int selinux_path_notify(const struct path *path, u64 mask,
>>    */
>>   static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>>   				     struct inode *inode, const char *name,
>> -				     void **buffer, bool alloc)
>> +				     bool alloc, void **buffer, u32 *len)
>>   {
>>   	u32 size;
>>   	int error;
>> @@ -3440,14 +3440,14 @@ static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>>   						&context, &size);
>>   	if (error)
>>   		return error;
>> -	error = size;
>> +	*len = size;
> 
> Depending on how you choose to resolve the variable type issue, you may
> be able to pass @len directly to security_sid_to_context().
>

Sounds great

>>   	if (alloc) {
>>   		*buffer = context;
>>   		goto out_nofree;
>>   	}
>>   	kfree(context);
>>   out_nofree:
>> -	return error;
>> +	return 0;
>>   }
> 
> 
> --
> paul-moore.com
> 
> 

Re: [PATCH v4 4/20] lsm: Refactor return value of LSM hook inode_listsecurity

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook inode_listsecurity to 0 or a negative error code.
>>
>> Before:
>> - Hook inode_listsecurity returns number of bytes used/required on
>>    success or a negative error code on failure.
>>
>> After:
>> - Hook inode_listsecurity returns 0 on success or a negative error
>>    code on failure. An output parameter @bytes is introduced to hold
>>    the number of bytes used/required on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   fs/nfs/nfs4proc.c             |  5 ++++-
>>   fs/xattr.c                    |  5 ++++-
>>   include/linux/lsm_hook_defs.h |  2 +-
>>   include/linux/security.h      |  7 ++++---
>>   net/socket.c                  |  9 +++++----
>>   security/security.c           | 29 +++++++++++++++++++++++++----
>>   security/selinux/hooks.c      |  8 +++++---
>>   security/smack/smack_lsm.c    |  6 ++++--
>>   8 files changed, 52 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index a691fa10b3e9..6d75758ba3d5 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -7848,10 +7848,13 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
>>   static ssize_t
>>   nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len)
>>   {
>> +	size_t bytes;
>>   	int len = 0;
>>   
>>   	if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) {
>> -		len = security_inode_listsecurity(inode, list, list_len);
>> +		len = security_inode_listsecurity(inode, list, list_len, &bytes);
>> +		if (!len)
>> +			len = bytes;
>>   		if (len >= 0 && list_len && len > list_len)
>>   			return -ERANGE;
>>   	}
> 
> See my comments below.
> 
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index f4e3bedf7272..ab7d7123a016 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
>> @@ -485,6 +485,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
>>   {
>>   	struct inode *inode = d_inode(dentry);
>>   	ssize_t error;
>> +	size_t bytes;
>>   
>>   	error = security_inode_listxattr(dentry);
>>   	if (error)
>> @@ -493,7 +494,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
>>   	if (inode->i_op->listxattr) {
>>   		error = inode->i_op->listxattr(dentry, list, size);
>>   	} else {
>> -		error = security_inode_listsecurity(inode, list, size);
>> +		error = security_inode_listsecurity(inode, list, size, &bytes);
>> +		if (!error)
>> +			error = bytes;
>>   		if (size && error > size)
>>   			error = -ERANGE;
> 
> More on this below, but since the buffer length is fixed we are
> already going to have to do a length comparison in the LSMs, why not
> do the check and return -ERANGE there?
>

Sounds great, thanks

>> diff --git a/net/socket.c b/net/socket.c
>> index e416920e9399..43f0e3c9a6e0 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -571,12 +571,13 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
>>   static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer,
>>   				size_t size)
>>   {
>> -	ssize_t len;
>> +	int err;
>> +	size_t len;
>>   	ssize_t used = 0;
>>   
>> -	len = security_inode_listsecurity(d_inode(dentry), buffer, size);
>> -	if (len < 0)
>> -		return len;
>> +	err = security_inode_listsecurity(d_inode(dentry), buffer, size, &len);
>> +	if (err < 0)
>> +		return err;
>>   	used += len;
>>   	if (buffer) {
>>   		if (size < used)
> 
> It doesn't show in the patch/diff, but if the LSM hook handles the length
> comparison we can simplify the -ERANGE code in sockfs_listxattr().
>

Will do

>> diff --git a/security/security.c b/security/security.c
>> index 614f14cbfff7..26eea8f4cd74 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2597,20 +2597,41 @@ int security_inode_setsecurity(struct inode *inode, const char *name,
>>    * @inode: inode
>>    * @buffer: buffer
>>    * @buffer_size: size of buffer
>> + * @bytes: number of bytes used/required
>>    *
>>    * Copy the extended attribute names for the security labels associated with
>>    * @inode into @buffer.  The maximum size of @buffer is specified by
>>    * @buffer_size.  @buffer may be NULL to request the size of the buffer
>>    * required.
>>    *
>> - * Return: Returns number of bytes used/required on success.
>> + * Return: Returns 0 on success or a negative error code on failure.
>>    */
>>   int security_inode_listsecurity(struct inode *inode,
>> -				char *buffer, size_t buffer_size)
>> +				char *buffer, size_t buffer_size,
>> +				size_t *bytes)
>>   {
>> +	int rc;
>> +	size_t used;
>> +	struct security_hook_list *hp;
>> +
>>   	if (unlikely(IS_PRIVATE(inode)))
>> -		return 0;
>> -	return call_int_hook(inode_listsecurity, inode, buffer, buffer_size);
>> +		return *bytes = 0;
>> +
>> +	used = 0;
>> +	hlist_for_each_entry(hp, &security_hook_heads.inode_listsecurity,
>> +			     list) {
>> +		rc = hp->hook.inode_listsecurity(inode, buffer, buffer_size,
>> +						 &used);
>> +		if (rc < 0)
>> +			return rc;
>> +		if (used != 0)
>> +			break;
>> +	}
>> +
>> +	*bytes = used;
>> +
>> +	return 0;
>> +
>>   }
>>   EXPORT_SYMBOL(security_inode_listsecurity);
> 
> For reasons associated with the static_call work, we really need to
> limit ourselves to the call_{int,void}_hook() macros on any new code.
> The good news is that I think we can do that here as the existing
> code isn't multi-LSM friendly.
>

Thanks for pointing that out. Good to know the current code is acceptable.

>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 70792bba24d9..5dedd3917d57 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3481,16 +3481,18 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>>   	return 0;
>>   }
>>   
>> -static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
>> +static int selinux_inode_listsecurity(struct inode *inode, char *buffer,
>> +				      size_t buffer_size, size_t *bytes)
>>   {
>>   	const int len = sizeof(XATTR_NAME_SELINUX);
>>   
>>   	if (!selinux_initialized())
>> -		return 0;
>> +		return *bytes = 0;
>>   
>>   	if (buffer && len <= buffer_size)
>>   		memcpy(buffer, XATTR_NAME_SELINUX, len);
>> -	return len;
>> +	*bytes = len;
>> +	return 0;
>>   }
> 
> Let's do something like below so we can catch -ERANGE in the LSMs
> themselves.
> 
>    if (!selinux_initialized())
>      return *bytes = 0;
> 
>    *bytes = sizeof(XATTR_NAME_SELINUX);
>    if (len > buffer_size);
>      return -ERANGE;
>    if (buffer)
>      memcpy(buffer, XATTR_NAME_SELINUX, *bytes);
>    
>    return 0;
>

Will do

>>   static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
>> index e7a5f6fd9a2d..6f73906bf7ea 100644
>> --- a/security/smack/smack_lsm.c
>> +++ b/security/smack/smack_lsm.c
>> @@ -1611,16 +1611,18 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap,
>>    * @inode: the object
>>    * @buffer: where they go
>>    * @buffer_size: size of buffer
>> + * @bytes: number of data bytes in buffer
>>    */
>>   static int smack_inode_listsecurity(struct inode *inode, char *buffer,
>> -				    size_t buffer_size)
>> +				    size_t buffer_size, size_t *bytes)
>>   {
>>   	int len = sizeof(XATTR_NAME_SMACK);
>>   
>>   	if (buffer != NULL && len <= buffer_size)
>>   		memcpy(buffer, XATTR_NAME_SMACK, len);
>>   
>> -	return len;
>> +	*bytes = len;
>> +	return 0;
>>   }
> 
> A similar approach could be used here.
>

Will do

> --
> paul-moore.com
> 

Re: [PATCH v4 5/20] lsm: Refactor return value of LSM hook inode_copy_up_xattr

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook inode_copy_up_xattr to 0 or a negative error code.
>>
>> Before:
>> - Hook inode_copy_up_xattr returns 0 when accepting xattr, 1 when
>>    discarding xattr, -EOPNOTSUPP if it does not know xattr, or any
>>    other negative error code otherwise.
>>
>> After:
>> - Hook inode_copy_up_xattr returns 0 when accepting xattr, *-ECANCELED*
>>    when discarding xattr, -EOPNOTSUPP if it does not know xattr, or
>>    any other negative error code otherwise.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   fs/overlayfs/copy_up.c            |  6 +++---
>>   security/integrity/evm/evm_main.c |  2 +-
>>   security/security.c               | 12 ++++++------
>>   security/selinux/hooks.c          |  4 ++--
>>   security/smack/smack_lsm.c        |  6 +++---
>>   5 files changed, 15 insertions(+), 15 deletions(-)
> 
> ...
> 
>> diff --git a/security/security.c b/security/security.c
>> index 26eea8f4cd74..12215ca286af 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2675,18 +2675,18 @@ EXPORT_SYMBOL(security_inode_copy_up);
>>    * lower layer to the union/overlay layer.   The caller is responsible for
>>    * reading and writing the xattrs, this hook is merely a filter.
>>    *
>> - * Return: Returns 0 to accept the xattr, 1 to discard the xattr, -EOPNOTSUPP
>> - *         if the security module does not know about attribute, or a negative
>> - *         error code to abort the copy up.
>> + * Return: Returns 0 to accept the xattr, -ECANCELED to discard the xattr,
>> + *         -EOPNOTSUPP if the security module does not know about attribute,
>> + *         or a negative error code to abort the copy up.
>>    */
>>   int security_inode_copy_up_xattr(struct dentry *src, const char *name)
>>   {
>>   	int rc;
>>   
>>   	/*
>> -	 * The implementation can return 0 (accept the xattr), 1 (discard the
>> -	 * xattr), -EOPNOTSUPP if it does not know anything about the xattr or
>> -	 * any other error code in case of an error.
>> +	 * The implementation can return 0 (accept the xattr), -ECANCELED
>> +	 * (discard the xattr), -EOPNOTSUPP if it does not know anything
>> +	 *  about the xattr or any other error code in case of an error.
>>   	 */
> 
> Updating the comment here is good, but considering that we also discuss
> the return value in the function header comment, I think it might be
> better to just remove this comment entirely and leave the function header
> comment as the single source.  Duplicated comments/docs tend to fall out
> of sync and create confusion.
>

OK, will do

>>   	rc = call_int_hook(inode_copy_up_xattr, src, name);
>>   	if (rc != LSM_RET_DEFAULT(inode_copy_up_xattr))
> 
> 
> --
> paul-moore.com
> 

Re: [PATCH v4 6/20] lsm: Refactor return value of LSM hook getselfattr

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook getselfattr to 0 or a negative error code.
>>
>> Before:
>> - Hook getselfattr returns number of attributes found on success
>>    or a negative error code on failure.
>>
>> After:
>> - Hook getselfattr returns 0 on success or a negative error code
>>    on failure. An output parameter @nattr is introduced to hold
>>    the number of attributes found on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   include/linux/lsm_hook_defs.h |  2 +-
>>   include/linux/security.h      |  5 +++--
>>   security/apparmor/lsm.c       |  5 +++--
>>   security/lsm_syscalls.c       |  6 +++++-
>>   security/security.c           | 18 +++++++++++-------
>>   security/selinux/hooks.c      | 13 +++++++++----
>>   security/smack/smack_lsm.c    | 13 +++++++++----
>>   7 files changed, 41 insertions(+), 21 deletions(-)
> 
> The getselfattr hook is different from the majority of the other LSM
> hooks as getselfattr is used as part of lsm_get_self_attr(2) syscall and
> not by other subsystems within the kernel.  Let's leave it as-is for now
> as it is sufficiently special case that a deviation is okay.
>

Got it, thanks

> --
> paul-moore.com
> 
> 

Re: [PATCH v4 8/20] lsm: Refactor return value of LSM hook getprocattr

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook getprocattr to 0 or a negative error code.
>>
>> Before:
>> - Hook getprocattr returns length of value on success or a negative
>>    error code on failure.
>>
>> After:
>> - Hook getprocattr returns 0 on success or a negative error code on
>>    failure. An output parameter @len is introduced to hold the
>>    length of value on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   fs/proc/base.c                |  5 ++++-
>>   include/linux/lsm_hook_defs.h |  2 +-
>>   include/linux/security.h      |  5 +++--
>>   security/apparmor/lsm.c       |  7 +++++--
>>   security/security.c           |  8 +++++---
>>   security/selinux/hooks.c      | 16 +++++++++-------
>>   security/smack/smack_lsm.c    | 11 ++++++-----
>>   7 files changed, 33 insertions(+), 21 deletions(-)
> 
> The patch 07/20 comments also apply here.
>

OK, will drop this patch.

> --
> paul-moore.com
> 
> 

Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook key_getsecurity

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook key_getsecurity to 0 or a negative error code.
>>
>> Before:
>> - Hook key_getsecurity returns length of value on success or a
>>    negative error code on failure.
>>
>> After:
>> - Hook key_getsecurity returns 0 on success or a negative error
>>    code on failure. An output parameter @len is introduced to hold
>>    the length of value on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   include/linux/lsm_hook_defs.h |  3 ++-
>>   include/linux/security.h      |  6 ++++--
>>   security/keys/keyctl.c        | 11 ++++++++---
>>   security/security.c           | 26 +++++++++++++++++++++-----
>>   security/selinux/hooks.c      | 11 +++++------
>>   security/smack/smack_lsm.c    | 21 +++++++++++----------
>>   6 files changed, 51 insertions(+), 27 deletions(-)
> 
> ...
> 
>> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>> index 4bc3e9398ee3..e9f857620f28 100644
>> --- a/security/keys/keyctl.c
>> +++ b/security/keys/keyctl.c
>> @@ -1565,6 +1565,7 @@ long keyctl_get_security(key_serial_t keyid,
>>   	struct key *key, *instkey;
>>   	key_ref_t key_ref;
>>   	char *context;
>> +	size_t len;
>>   	long ret;
>>   
>>   	key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
>> @@ -1586,15 +1587,18 @@ long keyctl_get_security(key_serial_t keyid,
>>   	}
>>   
>>   	key = key_ref_to_ptr(key_ref);
>> -	ret = security_key_getsecurity(key, &context);
>> -	if (ret == 0) {
>> +	ret = security_key_getsecurity(key, &context, &len);
>> +	if (ret < 0)
>> +		goto error;
> 
> Since there is already an if-else pattern here you might as well stick
> with that, for example:
> 
>    if (ret == 0) {
>      ...
>    } else if (ret > 0) {
>      ...
>    }
> 
> ... you should probably add a comment that @ret is -ERRNO on failure,
> but it doesn't look like you need an explicit test here since the error
> case will normally fall through to the 'error' label (which you shouldn't
> need anymore either).
>

Got it, thanks for pointing that out.

>> +	if (len == 0) {
>>   		/* if no information was returned, give userspace an empty
>>   		 * string */
>>   		ret = 1;
>>   		if (buffer && buflen > 0 &&
>>   		    copy_to_user(buffer, "", 1) != 0)
>>   			ret = -EFAULT;
>> -	} else if (ret > 0) {
>> +	} else {
>> +		ret = len;
>>   		/* return as much data as there's room for */
>>   		if (buffer && buflen > 0) {
>>   			if (buflen > ret)
>> @@ -1607,6 +1611,7 @@ long keyctl_get_security(key_serial_t keyid,
>>   		kfree(context);
>>   	}
>>   
>> +error:
>>   	key_ref_put(key_ref);
>>   	return ret;
>>   }
>> diff --git a/security/security.c b/security/security.c
>> index 9dd2ae6cf763..2c161101074d 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
>>    * security_key_getsecurity() - Get the key's security label
>>    * @key: key
>>    * @buffer: security label buffer
>> + * @len: the length of @buffer (including terminating NULL) on success
>>    *
>>    * Get a textual representation of the security context attached to a key for
>>    * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
>>    * storage for the NUL-terminated string and the caller should free it.
>>    *
>> - * Return: Returns the length of @buffer (including terminating NUL) or -ve if
>> - *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
>> - *         there is no security label assigned to the key.
>> + * Return: Returns 0 on success or -ve if an error occurs. May also return 0
>> + *         (and a NULL buffer pointer) if there is no security label assigned
>> + *         to the key.
>>    */
>> -int security_key_getsecurity(struct key *key, char **buffer)
>> +int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
>>   {
>> +	int rc;
>> +	size_t n = 0;
>> +	struct security_hook_list *hp;
>> +
>>   	*buffer = NULL;
>> -	return call_int_hook(key_getsecurity, key, buffer);
>> +
>> +	hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
>> +		rc = hp->hook.key_getsecurity(key, buffer, &n);
>> +		if (rc < 0)
>> +			return rc;
>> +		if (n)
>> +			break;
>> +	}
>> +
>> +	*len = n;
>> +
>> +	return 0;
>>   }
> 
> Help me understand why we can't continue to use the call_int_hook()
> macro here?
>

Before this patch, the hook may return +ve, 0, or -ve, and call_int_hook
breaks the loop when the hook return value is not 0.

After this patch, the +ve is stored in @n, so @n and return value should
both be checked to determine whether to break the loop. This is not
feasible with call_int_hook.

>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 16cd336aab3d..747ec602dec0 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6737,18 +6737,17 @@ static int selinux_key_permission(key_ref_t key_ref,
>>   	return avc_has_perm(sid, ksec->sid, SECCLASS_KEY, perm, NULL);
>>   }
>>   
>> -static int selinux_key_getsecurity(struct key *key, char **_buffer)
>> +static int selinux_key_getsecurity(struct key *key, char **_buffer,
>> +				   size_t *_len)
>>   {
>>   	struct key_security_struct *ksec = key->security;
>>   	char *context = NULL;
>> -	unsigned len;
>> +	u32 context_len;
> 
> Since @len doesn't collide with the parameter, you might as well just
> stick with @len as the local variable name.
>

Got it

>>   	int rc;
>>   
>> -	rc = security_sid_to_context(ksec->sid,
>> -				     &context, &len);
>> -	if (!rc)
>> -		rc = len;
>> +	rc = security_sid_to_context(ksec->sid, &context, &context_len);
>>   	*_buffer = context;
>> +	*_len = context_len;
>>   	return rc;
>>   }
> 
> --
> paul-moore.com
> 

Re: [PATCH v4 10/20] lsm: Refactor return value of LSM hook audit_rule_match

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook audit_rule_match to 0 or a negative error code.
>>
>> Before:
>> - Hook audit_rule_match returns 1 if the rule matches, 0 if it not,
>>    and negative error code otherwise.
>>
>> After:
>> - Hook audit_rule_match returns 0 on success or a negative error
>>    code on failure. An output parameter @match is introduced to hold
>>    the match result on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   include/linux/lsm_hook_defs.h     |  3 +-
>>   security/apparmor/audit.c         | 22 ++++++-------
>>   security/apparmor/include/audit.h |  2 +-
>>   security/security.c               | 15 ++++++++-
>>   security/selinux/include/audit.h  |  8 +++--
>>   security/selinux/ss/services.c    | 54 +++++++++++++++++--------------
>>   security/smack/smack_lsm.c        | 19 +++++++----
>>   7 files changed, 75 insertions(+), 48 deletions(-)
> 
> This is another odd hook, and similar to some of the others in this
> patchset, I'm not sure how applicable this would be to a BPF-based
> LSM.  I suspect you could safely block this from a BPF LSM and no one
> would notice or be upset.
> 
> However, if we did want to keep this hook for a BPF LSM, I think it
> might be better to encode the "match" results in the return value, just
> sticking with a more conventional 0/errno approach.  What do you think
> about 0:found/ok, -ENOENT:missing/ok, -ERRNO:other/error?  Yes, some
> of the existing LSM audit_match code uses -ENOENT but looking quickly
> at those error conditions it seems that we could consider them
> equivalent to a "missing" or "failed match" result and use -ENOENT for
> both.  If you're really not happy with that overloading, we could use
> something like -ENOMSG:missing/ok instead.
> 
> Thoughts?
>

I think we could just block it and see what happens.

> --
> paul-moore.com
> 

Re: [PATCH v4 7/20] lsm: Refactor return value of LSM hook setprocattr

[Xu Kuohai]

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook setprocattr to 0 or a negative error code.
>>
>> Before:
>> - Hook setprocattr returns the number of bytes written on success
>>    or a negative error code on failure.
>>
>> After:
>> - Hook setprocattr returns 0 on success or a negative error code
>>    on failure. An output parameter @wbytes is introduced to hold
>>    the number of bytes written on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   fs/proc/base.c                |  5 +++--
>>   include/linux/lsm_hook_defs.h |  3 ++-
>>   include/linux/security.h      |  5 +++--
>>   security/apparmor/lsm.c       | 10 +++++++---
>>   security/security.c           |  8 +++++---
>>   security/selinux/hooks.c      | 11 ++++++++---
>>   security/smack/smack_lsm.c    | 14 ++++++++++----
>>   7 files changed, 38 insertions(+), 18 deletions(-)
> 
> The security_setprocattr() hook is another odd case that we probably
> just want to leave alone for two reasons:
> 
> 1. With the move to LSM syscalls for getting/setting a task's LSM
> attributes we are "freezing" the procfs API and not adding any new
> entries to it.
> 
> 2. The BPF LSM doesn't currently register any procfs entries.
> 
> I'd suggest leaving security_setprocattr() as-is and blocking it in
> the BPF verifier, I can't see any reason why a BPF LSM would need
> this hook.
> 
OK, I'll drop this patch in the next version.

Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook key_getsecurity

[Paul Moore]

On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> On 7/19/2024 10:08 AM, Paul Moore wrote:
> > On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> >>
> >> To be consistent with most LSM hooks, convert the return value of
> >> hook key_getsecurity to 0 or a negative error code.
> >>
> >> Before:
> >> - Hook key_getsecurity returns length of value on success or a
> >>    negative error code on failure.
> >>
> >> After:
> >> - Hook key_getsecurity returns 0 on success or a negative error
> >>    code on failure. An output parameter @len is introduced to hold
> >>    the length of value on success.
> >>
> >> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> >> ---
> >>   include/linux/lsm_hook_defs.h |  3 ++-
> >>   include/linux/security.h      |  6 ++++--
> >>   security/keys/keyctl.c        | 11 ++++++++---
> >>   security/security.c           | 26 +++++++++++++++++++++-----
> >>   security/selinux/hooks.c      | 11 +++++------
> >>   security/smack/smack_lsm.c    | 21 +++++++++++----------
> >>   6 files changed, 51 insertions(+), 27 deletions(-)

...

> >> diff --git a/security/security.c b/security/security.c
> >> index 9dd2ae6cf763..2c161101074d 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
> >>    * security_key_getsecurity() - Get the key's security label
> >>    * @key: key
> >>    * @buffer: security label buffer
> >> + * @len: the length of @buffer (including terminating NULL) on success
> >>    *
> >>    * Get a textual representation of the security context attached to a key for
> >>    * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
> >>    * storage for the NUL-terminated string and the caller should free it.
> >>    *
> >> - * Return: Returns the length of @buffer (including terminating NUL) or -ve if
> >> - *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
> >> - *         there is no security label assigned to the key.
> >> + * Return: Returns 0 on success or -ve if an error occurs. May also return 0
> >> + *         (and a NULL buffer pointer) if there is no security label assigned
> >> + *         to the key.
> >>    */
> >> -int security_key_getsecurity(struct key *key, char **buffer)
> >> +int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
> >>   {
> >> +    int rc;
> >> +    size_t n = 0;
> >> +    struct security_hook_list *hp;
> >> +
> >>      *buffer = NULL;
> >> -    return call_int_hook(key_getsecurity, key, buffer);
> >> +
> >> +    hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
> >> +            rc = hp->hook.key_getsecurity(key, buffer, &n);
> >> +            if (rc < 0)
> >> +                    return rc;
> >> +            if (n)
> >> +                    break;
> >> +    }
> >> +
> >> +    *len = n;
> >> +
> >> +    return 0;
> >>   }
> >
> > Help me understand why we can't continue to use the call_int_hook()
> > macro here?
> >
>
> Before this patch, the hook may return +ve, 0, or -ve, and call_int_hook
> breaks the loop when the hook return value is not 0.
>
> After this patch, the +ve is stored in @n, so @n and return value should
> both be checked to determine whether to break the loop. This is not
> feasible with call_int_hook.

Yes, gotcha.  I was focused on the error condition and wasn't thinking
about the length getting zero'd out by a trailing callback.
Unfortunately, we *really* want to stick with the
call_{int,void}_hook() macros so I think we either need to find a way
to work within that constraint for existing macro callers, or we have
to leave this hook as-is for the moment.

-- 
paul-moore.com

Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook key_getsecurity

[Xu Kuohai]

On 7/23/2024 5:35 AM, Paul Moore wrote:
> On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>> On 7/19/2024 10:08 AM, Paul Moore wrote:
>>> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
>>>>
>>>> To be consistent with most LSM hooks, convert the return value of
>>>> hook key_getsecurity to 0 or a negative error code.
>>>>
>>>> Before:
>>>> - Hook key_getsecurity returns length of value on success or a
>>>>     negative error code on failure.
>>>>
>>>> After:
>>>> - Hook key_getsecurity returns 0 on success or a negative error
>>>>     code on failure. An output parameter @len is introduced to hold
>>>>     the length of value on success.
>>>>
>>>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>>>> ---
>>>>    include/linux/lsm_hook_defs.h |  3 ++-
>>>>    include/linux/security.h      |  6 ++++--
>>>>    security/keys/keyctl.c        | 11 ++++++++---
>>>>    security/security.c           | 26 +++++++++++++++++++++-----
>>>>    security/selinux/hooks.c      | 11 +++++------
>>>>    security/smack/smack_lsm.c    | 21 +++++++++++----------
>>>>    6 files changed, 51 insertions(+), 27 deletions(-)
> 
> ...
> 
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 9dd2ae6cf763..2c161101074d 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
>>>>     * security_key_getsecurity() - Get the key's security label
>>>>     * @key: key
>>>>     * @buffer: security label buffer
>>>> + * @len: the length of @buffer (including terminating NULL) on success
>>>>     *
>>>>     * Get a textual representation of the security context attached to a key for
>>>>     * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
>>>>     * storage for the NUL-terminated string and the caller should free it.
>>>>     *
>>>> - * Return: Returns the length of @buffer (including terminating NUL) or -ve if
>>>> - *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
>>>> - *         there is no security label assigned to the key.
>>>> + * Return: Returns 0 on success or -ve if an error occurs. May also return 0
>>>> + *         (and a NULL buffer pointer) if there is no security label assigned
>>>> + *         to the key.
>>>>     */
>>>> -int security_key_getsecurity(struct key *key, char **buffer)
>>>> +int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
>>>>    {
>>>> +    int rc;
>>>> +    size_t n = 0;
>>>> +    struct security_hook_list *hp;
>>>> +
>>>>       *buffer = NULL;
>>>> -    return call_int_hook(key_getsecurity, key, buffer);
>>>> +
>>>> +    hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
>>>> +            rc = hp->hook.key_getsecurity(key, buffer, &n);
>>>> +            if (rc < 0)
>>>> +                    return rc;
>>>> +            if (n)
>>>> +                    break;
>>>> +    }
>>>> +
>>>> +    *len = n;
>>>> +
>>>> +    return 0;
>>>>    }
>>>
>>> Help me understand why we can't continue to use the call_int_hook()
>>> macro here?
>>>
>>
>> Before this patch, the hook may return +ve, 0, or -ve, and call_int_hook
>> breaks the loop when the hook return value is not 0.
>>
>> After this patch, the +ve is stored in @n, so @n and return value should
>> both be checked to determine whether to break the loop. This is not
>> feasible with call_int_hook.
> 
> Yes, gotcha.  I was focused on the error condition and wasn't thinking
> about the length getting zero'd out by a trailing callback.
> Unfortunately, we *really* want to stick with the
> call_{int,void}_hook() macros so I think we either need to find a way
> to work within that constraint for existing macro callers, or we have
> to leave this hook as-is for the moment.
> 

Let's leave it as is. So we ultimately have four hooks that can be
converted, two of which require adding additional output parameter to
hold odd return values. These output parameters require extra work
on the BPF verifier, and it doesn't seem worthwhile just for two
hooks. So I prefer to keep only the two patches that handle
conversion without adding output parameters (patch 1 and 5).

Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook key_getsecurity

[Paul Moore]

On Tue, Jul 23, 2024 at 3:04 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> On 7/23/2024 5:35 AM, Paul Moore wrote:
> > On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> >> On 7/19/2024 10:08 AM, Paul Moore wrote:
> >>> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> >>>>
> >>>> To be consistent with most LSM hooks, convert the return value of
> >>>> hook key_getsecurity to 0 or a negative error code.
> >>>>
> >>>> Before:
> >>>> - Hook key_getsecurity returns length of value on success or a
> >>>>     negative error code on failure.
> >>>>
> >>>> After:
> >>>> - Hook key_getsecurity returns 0 on success or a negative error
> >>>>     code on failure. An output parameter @len is introduced to hold
> >>>>     the length of value on success.
> >>>>
> >>>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> >>>> ---
> >>>>    include/linux/lsm_hook_defs.h |  3 ++-
> >>>>    include/linux/security.h      |  6 ++++--
> >>>>    security/keys/keyctl.c        | 11 ++++++++---
> >>>>    security/security.c           | 26 +++++++++++++++++++++-----
> >>>>    security/selinux/hooks.c      | 11 +++++------
> >>>>    security/smack/smack_lsm.c    | 21 +++++++++++----------
> >>>>    6 files changed, 51 insertions(+), 27 deletions(-)
> >
> > ...
> >
> >>>> diff --git a/security/security.c b/security/security.c
> >>>> index 9dd2ae6cf763..2c161101074d 100644
> >>>> --- a/security/security.c
> >>>> +++ b/security/security.c
> >>>> @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
> >>>>     * security_key_getsecurity() - Get the key's security label
> >>>>     * @key: key
> >>>>     * @buffer: security label buffer
> >>>> + * @len: the length of @buffer (including terminating NULL) on success
> >>>>     *
> >>>>     * Get a textual representation of the security context attached to a key for
> >>>>     * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
> >>>>     * storage for the NUL-terminated string and the caller should free it.
> >>>>     *
> >>>> - * Return: Returns the length of @buffer (including terminating NUL) or -ve if
> >>>> - *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
> >>>> - *         there is no security label assigned to the key.
> >>>> + * Return: Returns 0 on success or -ve if an error occurs. May also return 0
> >>>> + *         (and a NULL buffer pointer) if there is no security label assigned
> >>>> + *         to the key.
> >>>>     */
> >>>> -int security_key_getsecurity(struct key *key, char **buffer)
> >>>> +int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
> >>>>    {
> >>>> +    int rc;
> >>>> +    size_t n = 0;
> >>>> +    struct security_hook_list *hp;
> >>>> +
> >>>>       *buffer = NULL;
> >>>> -    return call_int_hook(key_getsecurity, key, buffer);
> >>>> +
> >>>> +    hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
> >>>> +            rc = hp->hook.key_getsecurity(key, buffer, &n);
> >>>> +            if (rc < 0)
> >>>> +                    return rc;
> >>>> +            if (n)
> >>>> +                    break;
> >>>> +    }
> >>>> +
> >>>> +    *len = n;
> >>>> +
> >>>> +    return 0;
> >>>>    }
> >>>
> >>> Help me understand why we can't continue to use the call_int_hook()
> >>> macro here?
> >>>
> >>
> >> Before this patch, the hook may return +ve, 0, or -ve, and call_int_hook
> >> breaks the loop when the hook return value is not 0.
> >>
> >> After this patch, the +ve is stored in @n, so @n and return value should
> >> both be checked to determine whether to break the loop. This is not
> >> feasible with call_int_hook.
> >
> > Yes, gotcha.  I was focused on the error condition and wasn't thinking
> > about the length getting zero'd out by a trailing callback.
> > Unfortunately, we *really* want to stick with the
> > call_{int,void}_hook() macros so I think we either need to find a way
> > to work within that constraint for existing macro callers, or we have
> > to leave this hook as-is for the moment.
> >
>
> Let's leave it as is. So we ultimately have four hooks that can be
> converted, two of which require adding additional output parameter to
> hold odd return values. These output parameters require extra work
> on the BPF verifier, and it doesn't seem worthwhile just for two
> hooks. So I prefer to keep only the two patches that handle
> conversion without adding output parameters (patch 1 and 5).

Fair enough.  Thanks for working on this, between the changes to the
LSM framework and the BPF verifier, I think this is still a nice
improvement.

-- 
paul-moore.com