[PATCH] security, lsm: security_old_inode_init_security() Handle multi LSM registration

* [PATCH] security, lsm: security_old_inode_init_security() Handle multi LSM registration
@ 2023-04-01 21:41 Valentin Vidic
  2023-04-02 15:14 ` Paul Moore
  0 siblings, 1 reply; 5+ messages in thread
From: Valentin Vidic @ 2023-04-01 21:41 UTC (permalink / raw)
  To: Paul Moore
  Cc: James Morris, Serge E. Hallyn, Christian Brauner, Dave Chinner,
	Casey Schaufler, John Johansen, Micah Morton, Günther Noack,
	Al Viro, Valentin Vidic, Mickaël Salaün, linux-kernel,
	linux-security-module, Vivek Goyal

Copying files to an ocfs2 filesystem causes a crash with NULL pointer
dereference in strlen.

[   27.386786] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   27.386818] #PF: supervisor read access in kernel mode
[   27.386832] #PF: error_code(0x0000) - not-present page
[   27.386844] PGD 0 P4D 0=20
[   27.386855] Oops: 0000 [#1] PREEMPT SMP PTI
[   27.386867] CPU: 0 PID: 1792 Comm: cp Not tainted 6.1.0-5-amd64 #1  Debian 6.1.12-1
[   27.386887] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014
[   27.386904] RIP: 0010:strlen+0x0/0x20
[   27.386928] Code: b6 07 38 d0 74 14 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 c3 cc cc cc cc 48 89 f8 c3 cc cc cc cc 0f 1f 84 00 00 00 00 00 <80> 3f 00 74 14 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 cc
[   27.386966] RSP: 0018:ffffa33340e4fbc0 EFLAGS: 00010202
[   27.386980] RAX: ffff8b578c3b1800 RBX: 0000000000000001 RCX: 0000000000000000
[   27.386996] RDX: 0000000000000100 RSI: ffff8b57843d86e8 RDI: 0000000000000000
[   27.387012] RBP: ffff8b57849ca608 R08: ffffa33340e4fc7c R09: ffffa33340e4fc84
[   27.387027] R10: ffff8b578f1e6000 R11: ffffa33340e4fc80 R12: ffffa33340e4fcb8
[   27.387043] R13: ffffa33340e4fc84 R14: 00000000000041c0 R15: ffffa33340e4fc7c
[   27.387059] FS:  00007f7b36d50500(0000) GS:ffff8b57bec00000(0000) knlGS:0000000000000000
[   27.387077] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   27.387091] CR2: 0000000000000000 CR3: 000000003cfe2003 CR4: 0000000000370ef0
[   27.387111] Call Trace:
[   27.387130]  <TASK>
[   27.387141]  ocfs2_calc_xattr_init+0x7d/0x330 [ocfs2]
[   27.387382]  ocfs2_mknod+0x471/0x1020 [ocfs2]
[   27.387471]  ? preempt_count_add+0x6a/0xa0
[   27.387487]  ? _raw_spin_lock+0x13/0x40
[   27.387506]  ocfs2_mkdir+0x44/0x130 [ocfs2]
[   27.387583]  ? security_inode_mkdir+0x3e/0x70
[   27.387598]  vfs_mkdir+0x9c/0x140
[   27.387617]  do_mkdirat+0x142/0x170
[   27.387631]  __x64_sys_mkdirat+0x47/0x80
[   27.387643]  do_syscall_64+0x58/0xc0
[   27.387659]  ? vfs_fstatat+0x5b/0x70
[   27.387671]  ? __do_sys_newfstatat+0x3f/0x80
[   27.387684]  ? fpregs_assert_state_consistent+0x22/0x50
[   27.387698]  ? exit_to_user_mode_prepare+0x3c/0x1c0
[   27.387712]  ? syscall_exit_to_user_mode+0x17/0x40
[   27.387726]  ? do_syscall_64+0x67/0xc0
[   27.387738]  ? exit_to_user_mode_prepare+0x3c/0x1c0
[   27.387752]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Similar to security_dentry_init_security fix in 7f5056b9e7b, the problem
is that ocfs2 checks the return code from security_old_inode_init_security
and if the return code is 0, it assumes everything is fine and continues
to call strlen(name), which crashes.

Typically SELinux LSM returns 0 and sets name to "security.selinux" and
it is not a problem. Or if SELinux is not compiled in or disabled, it
returns -EOPNOTSUP and ocfs2 deals with it.

However if BPF LSM is enabled, it registeres every hook and returns the
default return value, in this case 0.

This patch copies the behaviour of security_dentry_init_security() to
allow only one LSM to initialize security context (or return the default
value of -EOPNOTSUP).

Signed-off-by: Valentin Vidic <vvidic@valentin-vidic.from.hr>
---
 include/linux/lsm_hook_defs.h |  2 +-
 security/security.c           | 16 ++++++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 094b76dc7164..ea152b6da56b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -111,7 +111,7 @@ LSM_HOOK(int, 0, path_notify, const struct path *path, u64 mask,
 	 unsigned int obj_type)
 LSM_HOOK(int, 0, inode_alloc_security, struct inode *inode)
 LSM_HOOK(void, LSM_RET_VOID, inode_free_security, struct inode *inode)
-LSM_HOOK(int, 0, inode_init_security, struct inode *inode,
+LSM_HOOK(int, -EOPNOTSUPP, inode_init_security, struct inode *inode,
 	 struct inode *dir, const struct qstr *qstr, const char **name,
 	 void **value, size_t *len)
 LSM_HOOK(int, 0, inode_init_security_anon, struct inode *inode,
diff --git a/security/security.c b/security/security.c
index cf6cc576736f..a25d84950a97 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1164,10 +1164,22 @@ int security_old_inode_init_security(struct inode *inode, struct inode *dir,
 				     const struct qstr *qstr, const char **name,
 				     void **value, size_t *len)
 {
+	struct security_hook_list *hp;
+	int rc;
+
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
-	return call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir,
-			     qstr, name, value, len);
+
+	/*
+	 * Only one module will provide a security context.
+	 */
+	hlist_for_each_entry(hp, &security_hook_heads.inode_init_security, list) {
+		rc = hp->hook.inode_init_security(inode, dir, qstr, name,
+						  value, len);
+		if (rc != LSM_RET_DEFAULT(inode_init_security))
+			return rc;
+	}
+	return LSM_RET_DEFAULT(inode_init_security);
 }
 EXPORT_SYMBOL(security_old_inode_init_security);
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread