Re: "kernel NULL pointer dereference" crash when attempting a write

Re: "kernel NULL pointer dereference" crash when attempting a write

[Paul Moore]

On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
>
> Hello,
>
> I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
>
> This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
>
> /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> distro / arch: Arch Linux / x86_64
> SELinux is not enabled
> ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
>
> relevant dmesg output:
> [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [   30.947206] #PF: supervisor read access in kernel mode
> [   30.947258] #PF: error_code(0x0000) - not-present page
> [   30.947310] PGD 0 P4D 0
> [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> [   30.947569] RIP: 0010:strlen+0x0/0x20
> [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> [   30.948376] Call Trace:
> [   30.948404]  <TASK>
> [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]

My guess is that the "name_len = strlen(name);" line in
ceph_security_init_secctx() is causing the problem as "name" is likely
NULL/garbage without any LSMs configured for that hook.

[SIDE NOTE: we should remove the comment block directly above that
line as it no longer applies, arguably commit 15bf32398ad4 should have
removed it.]

I'm open to suggestions on the best approach, but my gut feeling is
that the fix is to have the security_dentry_init_security() hook set
the xattr_name parameter to NULL before calling into the LSMs and then
the callers should check for "name == NULL" on return.  In the case of
ceph that would probably be something like this:

  name_len = (name ? strlen(name) : 0);

... with fuse looking very similar, although fuse appears to need a
"strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
doesn't use the xattr name.

Thoughts?

-- 
paul moore
paul-moore.com

Re: "kernel NULL pointer dereference" crash when attempting a write

[Jeff Layton]

On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > 
> > Hello,
> > 
> > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > 
> > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > 
> > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > distro / arch: Arch Linux / x86_64
> > SELinux is not enabled
> > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > 
> > relevant dmesg output:
> > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [   30.947206] #PF: supervisor read access in kernel mode
> > [   30.947258] #PF: error_code(0x0000) - not-present page
> > [   30.947310] PGD 0 P4D 0
> > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > [   30.948376] Call Trace:
> > [   30.948404]  <TASK>
> > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> 
> My guess is that the "name_len = strlen(name);" line in
> ceph_security_init_secctx() is causing the problem as "name" is likely
> NULL/garbage without any LSMs configured for that hook.
> 
> [SIDE NOTE: we should remove the comment block directly above that
> line as it no longer applies, arguably commit 15bf32398ad4 should have
> removed it.]
> 
> I'm open to suggestions on the best approach, but my gut feeling is
> that the fix is to have the security_dentry_init_security() hook set
> the xattr_name parameter to NULL before calling into the LSMs and then
> the callers should check for "name == NULL" on return.  In the case of
> ceph that would probably be something like this:
> 
>   name_len = (name ? strlen(name) : 0);
> 
> ... with fuse looking very similar, although fuse appears to need a
> "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> doesn't use the xattr name.
> 
> Thoughts?
> 

Actually, if we don't get an xattr name back from the call, we don't
need to do anything in ceph_security_init_secctx(). Stephen, could you
test this patch and let us know if it fixes it?

-------------------8<--------------------

[PATCH] ceph: when no LSM is configured, don't set security xattr

If the xattr name pointer is not populated after the call to
security_dentry_init_security(), then we should assume that no LSM is
configured and that no xattr needs to be set for it. Also, remove a
now-defunct comment.

Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
Cc: Vivek Goyal <vgoyal@redhat.com>
Reported-by: Stephen Muth <smuth4@gmail.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/xattr.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
index fcf7dfdecf96..a61e1adb49a9 100644
--- a/fs/ceph/xattr.c
+++ b/fs/ceph/xattr.c
@@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 			   struct ceph_acl_sec_ctx *as_ctx)
 {
 	struct ceph_pagelist *pagelist = as_ctx->pagelist;
-	const char *name;
+	const char *name = NULL;
 	size_t name_len;
 	int err;
 
@@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 		goto out;
 	}
 
+	/* No LSM configured? Do nothing. */
+	if (!name) {
+		err = 0;
+		goto out;
+	}
+
 	err = -ENOMEM;
 	if (!pagelist) {
 		pagelist = ceph_pagelist_alloc(GFP_KERNEL);
@@ -1330,11 +1336,6 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
 		ceph_pagelist_encode_32(pagelist, 1);
 	}
 
-	/*
-	 * FIXME: Make security_dentry_init_security() generic. Currently
-	 * It only supports single security module and only selinux has
-	 * dentry_init_security hook.
-	 */
 	name_len = strlen(name);
 	err = ceph_pagelist_reserve(pagelist,
 				    4 * 2 + name_len + as_ctx->sec_ctxlen);
-- 
2.34.1

Re: "kernel NULL pointer dereference" crash when attempting a write

[Christian Brauner]

On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> > On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > > 
> > > Hello,
> > > 
> > > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > > 
> > > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > > 
> > > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > > distro / arch: Arch Linux / x86_64
> > > SELinux is not enabled
> > > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > > 
> > > relevant dmesg output:
> > > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > [   30.947206] #PF: supervisor read access in kernel mode
> > > [   30.947258] #PF: error_code(0x0000) - not-present page
> > > [   30.947310] PGD 0 P4D 0
> > > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > > [   30.948376] Call Trace:
> > > [   30.948404]  <TASK>
> > > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> > 
> > My guess is that the "name_len = strlen(name);" line in
> > ceph_security_init_secctx() is causing the problem as "name" is likely
> > NULL/garbage without any LSMs configured for that hook.
> > 
> > [SIDE NOTE: we should remove the comment block directly above that
> > line as it no longer applies, arguably commit 15bf32398ad4 should have
> > removed it.]
> > 
> > I'm open to suggestions on the best approach, but my gut feeling is
> > that the fix is to have the security_dentry_init_security() hook set
> > the xattr_name parameter to NULL before calling into the LSMs and then
> > the callers should check for "name == NULL" on return.  In the case of
> > ceph that would probably be something like this:
> > 
> >   name_len = (name ? strlen(name) : 0);
> > 
> > ... with fuse looking very similar, although fuse appears to need a
> > "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> > doesn't use the xattr name.
> > 
> > Thoughts?
> > 
> 
> Actually, if we don't get an xattr name back from the call, we don't
> need to do anything in ceph_security_init_secctx(). Stephen, could you
> test this patch and let us know if it fixes it?
> 
> -------------------8<--------------------
> 
> [PATCH] ceph: when no LSM is configured, don't set security xattr
> 
> If the xattr name pointer is not populated after the call to
> security_dentry_init_security(), then we should assume that no LSM is
> configured and that no xattr needs to be set for it. Also, remove a
> now-defunct comment.
> 
> Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Reported-by: Stephen Muth <smuth4@gmail.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/xattr.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index fcf7dfdecf96..a61e1adb49a9 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
>  			   struct ceph_acl_sec_ctx *as_ctx)
>  {
>  	struct ceph_pagelist *pagelist = as_ctx->pagelist;
> -	const char *name;
> +	const char *name = NULL;
>  	size_t name_len;
>  	int err;
>  
> @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
>  		goto out;
>  	}
>  
> +	/* No LSM configured? Do nothing. */
> +	if (!name) {

Excuse my french but that reads very riské. Can we somehow make
security_dentry_init_security() return an error number that caller's
like ceph can use to skip?
I think that returning 0 from security_dentry_init_security() and having
*name be returned as NULL is very fragile.

Re: "kernel NULL pointer dereference" crash when attempting a write

[Jeff Layton]

On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
> On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> > On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> > > On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > > > 
> > > > Hello,
> > > > 
> > > > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > > > 
> > > > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > > > 
> > > > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > > > distro / arch: Arch Linux / x86_64
> > > > SELinux is not enabled
> > > > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > > > 
> > > > relevant dmesg output:
> > > > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > [   30.947206] #PF: supervisor read access in kernel mode
> > > > [   30.947258] #PF: error_code(0x0000) - not-present page
> > > > [   30.947310] PGD 0 P4D 0
> > > > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > > > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > > > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > > > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > > > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > > > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > > > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > > > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > > > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > > > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > > > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > > > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > > > [   30.948376] Call Trace:
> > > > [   30.948404]  <TASK>
> > > > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> > > 
> > > My guess is that the "name_len = strlen(name);" line in
> > > ceph_security_init_secctx() is causing the problem as "name" is likely
> > > NULL/garbage without any LSMs configured for that hook.
> > > 
> > > [SIDE NOTE: we should remove the comment block directly above that
> > > line as it no longer applies, arguably commit 15bf32398ad4 should have
> > > removed it.]
> > > 
> > > I'm open to suggestions on the best approach, but my gut feeling is
> > > that the fix is to have the security_dentry_init_security() hook set
> > > the xattr_name parameter to NULL before calling into the LSMs and then
> > > the callers should check for "name == NULL" on return.  In the case of
> > > ceph that would probably be something like this:
> > > 
> > >   name_len = (name ? strlen(name) : 0);
> > > 
> > > ... with fuse looking very similar, although fuse appears to need a
> > > "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> > > doesn't use the xattr name.
> > > 
> > > Thoughts?
> > > 
> > 
> > Actually, if we don't get an xattr name back from the call, we don't
> > need to do anything in ceph_security_init_secctx(). Stephen, could you
> > test this patch and let us know if it fixes it?
> > 
> > -------------------8<--------------------
> > 
> > [PATCH] ceph: when no LSM is configured, don't set security xattr
> > 
> > If the xattr name pointer is not populated after the call to
> > security_dentry_init_security(), then we should assume that no LSM is
> > configured and that no xattr needs to be set for it. Also, remove a
> > now-defunct comment.
> > 
> > Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
> > Cc: Vivek Goyal <vgoyal@redhat.com>
> > Reported-by: Stephen Muth <smuth4@gmail.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/xattr.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > index fcf7dfdecf96..a61e1adb49a9 100644
> > --- a/fs/ceph/xattr.c
> > +++ b/fs/ceph/xattr.c
> > @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> >  			   struct ceph_acl_sec_ctx *as_ctx)
> >  {
> >  	struct ceph_pagelist *pagelist = as_ctx->pagelist;
> > -	const char *name;
> > +	const char *name = NULL;
> >  	size_t name_len;
> >  	int err;
> >  
> > @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> >  		goto out;
> >  	}
> >  
> > +	/* No LSM configured? Do nothing. */
> > +	if (!name) {
> 
> Excuse my french but that reads very riské. Can we somehow make
> security_dentry_init_security() return an error number that caller's
> like ceph can use to skip?
> I think that returning 0 from security_dentry_init_security() and having
> *name be returned as NULL is very fragile.

Good point. In fact, I'm not sure why he didn't get an -EOPNOTSUPP
return here:

        return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
                                name, xattr_name, ctx, ctxlen);

It looks like the second argument is what should end up being returned
when there are no entries in the security_hook_list. So it seems like
maybe Stephen's security_hook_list was not empty, but didn't have a
vector for dentry_init_security and returned the default 0 there?
-- 
Jeff Layton <jlayton@kernel.org>

Re: "kernel NULL pointer dereference" crash when attempting a write

[Christian Brauner]

On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
> On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
> > On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> > > On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> > > > On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > > > > 
> > > > > Hello,
> > > > > 
> > > > > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > > > > 
> > > > > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > > > > 
> > > > > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > > > > distro / arch: Arch Linux / x86_64
> > > > > SELinux is not enabled
> > > > > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > > > > 
> > > > > relevant dmesg output:
> > > > > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > > [   30.947206] #PF: supervisor read access in kernel mode
> > > > > [   30.947258] #PF: error_code(0x0000) - not-present page
> > > > > [   30.947310] PGD 0 P4D 0
> > > > > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > > > > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > > > > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > > > > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > > > > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > > > > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > > > > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > > > > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > > > > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > > > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > > > > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > > > > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > > > > [   30.948376] Call Trace:
> > > > > [   30.948404]  <TASK>
> > > > > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> > > > 
> > > > My guess is that the "name_len = strlen(name);" line in
> > > > ceph_security_init_secctx() is causing the problem as "name" is likely
> > > > NULL/garbage without any LSMs configured for that hook.
> > > > 
> > > > [SIDE NOTE: we should remove the comment block directly above that
> > > > line as it no longer applies, arguably commit 15bf32398ad4 should have
> > > > removed it.]
> > > > 
> > > > I'm open to suggestions on the best approach, but my gut feeling is
> > > > that the fix is to have the security_dentry_init_security() hook set
> > > > the xattr_name parameter to NULL before calling into the LSMs and then
> > > > the callers should check for "name == NULL" on return.  In the case of
> > > > ceph that would probably be something like this:
> > > > 
> > > >   name_len = (name ? strlen(name) : 0);
> > > > 
> > > > ... with fuse looking very similar, although fuse appears to need a
> > > > "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> > > > doesn't use the xattr name.
> > > > 
> > > > Thoughts?
> > > > 
> > > 
> > > Actually, if we don't get an xattr name back from the call, we don't
> > > need to do anything in ceph_security_init_secctx(). Stephen, could you
> > > test this patch and let us know if it fixes it?
> > > 
> > > -------------------8<--------------------
> > > 
> > > [PATCH] ceph: when no LSM is configured, don't set security xattr
> > > 
> > > If the xattr name pointer is not populated after the call to
> > > security_dentry_init_security(), then we should assume that no LSM is
> > > configured and that no xattr needs to be set for it. Also, remove a
> > > now-defunct comment.
> > > 
> > > Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
> > > Cc: Vivek Goyal <vgoyal@redhat.com>
> > > Reported-by: Stephen Muth <smuth4@gmail.com>
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/ceph/xattr.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > index fcf7dfdecf96..a61e1adb49a9 100644
> > > --- a/fs/ceph/xattr.c
> > > +++ b/fs/ceph/xattr.c
> > > @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > >  			   struct ceph_acl_sec_ctx *as_ctx)
> > >  {
> > >  	struct ceph_pagelist *pagelist = as_ctx->pagelist;
> > > -	const char *name;
> > > +	const char *name = NULL;
> > >  	size_t name_len;
> > >  	int err;
> > >  
> > > @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > >  		goto out;
> > >  	}
> > >  
> > > +	/* No LSM configured? Do nothing. */
> > > +	if (!name) {
> > 
> > Excuse my french but that reads very riské. Can we somehow make
> > security_dentry_init_security() return an error number that caller's
> > like ceph can use to skip?
> > I think that returning 0 from security_dentry_init_security() and having
> > *name be returned as NULL is very fragile.
> 
> Good point. In fact, I'm not sure why he didn't get an -EOPNOTSUPP
> return here:
> 
>         return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>                                 name, xattr_name, ctx, ctxlen);
> 
> It looks like the second argument is what should end up being returned
> when there are no entries in the security_hook_list. So it seems like
> maybe Stephen's security_hook_list was not empty, but didn't have a
> vector for dentry_init_security and returned the default 0 there?

Yeah, I came to the exact same analysis and subsequent question in:
https://lore.kernel.org/ceph-devel/20220125111111.irvr4dg3i56crqrs@wittgenstein

It seems to me that selinux support is compiled in in archlinux but no
policy loaded/enabled. I don't know if that means that the hooks is
called and by default returns 0 or something. I'm rather confused as the
call_int_hook() implies -EOPNOTSUPP is returned but looking at
dentry_init_security as defined in LSM_HOOKS(int, 0,
dentry_init_security, [...]) in include/linux/lsm_hook_defs.h implies
that the default return value is 0.

Re: "kernel NULL pointer dereference" crash when attempting a write

[Jeff Layton]

On Tue, 2022-01-25 at 13:12 +0100, Christian Brauner wrote:
> On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
> > On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
> > > On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> > > > On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> > > > > On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > > > > > 
> > > > > > Hello,
> > > > > > 
> > > > > > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > > > > > 
> > > > > > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > > > > > 
> > > > > > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > > > > > distro / arch: Arch Linux / x86_64
> > > > > > SELinux is not enabled
> > > > > > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > > > > > 
> > > > > > relevant dmesg output:
> > > > > > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > > > [   30.947206] #PF: supervisor read access in kernel mode
> > > > > > [   30.947258] #PF: error_code(0x0000) - not-present page
> > > > > > [   30.947310] PGD 0 P4D 0
> > > > > > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > > > > > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > > > > > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > > > > > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > > > > > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > > > > > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > > > > > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > > > > > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > > > > > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > > > > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > > > > > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > > > > > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > > > > > [   30.948376] Call Trace:
> > > > > > [   30.948404]  <TASK>
> > > > > > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> > > > > 
> > > > > My guess is that the "name_len = strlen(name);" line in
> > > > > ceph_security_init_secctx() is causing the problem as "name" is likely
> > > > > NULL/garbage without any LSMs configured for that hook.
> > > > > 
> > > > > [SIDE NOTE: we should remove the comment block directly above that
> > > > > line as it no longer applies, arguably commit 15bf32398ad4 should have
> > > > > removed it.]
> > > > > 
> > > > > I'm open to suggestions on the best approach, but my gut feeling is
> > > > > that the fix is to have the security_dentry_init_security() hook set
> > > > > the xattr_name parameter to NULL before calling into the LSMs and then
> > > > > the callers should check for "name == NULL" on return.  In the case of
> > > > > ceph that would probably be something like this:
> > > > > 
> > > > >   name_len = (name ? strlen(name) : 0);
> > > > > 
> > > > > ... with fuse looking very similar, although fuse appears to need a
> > > > > "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> > > > > doesn't use the xattr name.
> > > > > 
> > > > > Thoughts?
> > > > > 
> > > > 
> > > > Actually, if we don't get an xattr name back from the call, we don't
> > > > need to do anything in ceph_security_init_secctx(). Stephen, could you
> > > > test this patch and let us know if it fixes it?
> > > > 
> > > > -------------------8<--------------------
> > > > 
> > > > [PATCH] ceph: when no LSM is configured, don't set security xattr
> > > > 
> > > > If the xattr name pointer is not populated after the call to
> > > > security_dentry_init_security(), then we should assume that no LSM is
> > > > configured and that no xattr needs to be set for it. Also, remove a
> > > > now-defunct comment.
> > > > 
> > > > Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
> > > > Cc: Vivek Goyal <vgoyal@redhat.com>
> > > > Reported-by: Stephen Muth <smuth4@gmail.com>
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > ---
> > > >  fs/ceph/xattr.c | 13 +++++++------
> > > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > index fcf7dfdecf96..a61e1adb49a9 100644
> > > > --- a/fs/ceph/xattr.c
> > > > +++ b/fs/ceph/xattr.c
> > > > @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > >  			   struct ceph_acl_sec_ctx *as_ctx)
> > > >  {
> > > >  	struct ceph_pagelist *pagelist = as_ctx->pagelist;
> > > > -	const char *name;
> > > > +	const char *name = NULL;
> > > >  	size_t name_len;
> > > >  	int err;
> > > >  
> > > > @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > >  		goto out;
> > > >  	}
> > > >  
> > > > +	/* No LSM configured? Do nothing. */
> > > > +	if (!name) {
> > > 
> > > Excuse my french but that reads very riské. Can we somehow make
> > > security_dentry_init_security() return an error number that caller's
> > > like ceph can use to skip?
> > > I think that returning 0 from security_dentry_init_security() and having
> > > *name be returned as NULL is very fragile.
> > 
> > Good point. In fact, I'm not sure why he didn't get an -EOPNOTSUPP
> > return here:
> > 
> >         return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> >                                 name, xattr_name, ctx, ctxlen);
> > 
> > It looks like the second argument is what should end up being returned
> > when there are no entries in the security_hook_list. So it seems like
> > maybe Stephen's security_hook_list was not empty, but didn't have a
> > vector for dentry_init_security and returned the default 0 there?
> 
> Yeah, I came to the exact same analysis and subsequent question in:
> https://lore.kernel.org/ceph-devel/20220125111111.irvr4dg3i56crqrs@wittgenstein
> 
> It seems to me that selinux support is compiled in in archlinux but no
> policy loaded/enabled. I don't know if that means that the hooks is
> called and by default returns 0 or something. I'm rather confused as the
> call_int_hook() implies -EOPNOTSUPP is returned but looking at
> dentry_init_security as defined in LSM_HOOKS(int, 0,
> dentry_init_security, [...]) in include/linux/lsm_hook_defs.h implies
> that the default return value is 0.

Yeah, the problem is the call_int_hook macro, I think, which wants to
walk through all of the LSMs in the stack and call this for all of
them.  What happens though if you have 2 entries with valid
dentry_init_security hooks in them, and they both try to set the xattr
name?

This can only ever return a single xattr_name, so maybe the right thing
to do is to make the default for the LSM_HOOK be -EOPNOTSUPP and add a
macro that does call_int_hook, but returns after the first supported
hook call? Something like this maybe? (untested):

-----------8<----------------

[RFC PATCH] security: dentry_init_security hook should return on first success

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 include/linux/lsm_hook_defs.h |  2 +-
 security/security.c           | 18 ++++++++++++++++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index a5a724c308d8..819ec92dc2a8 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, const struct super_block *oldsb,
 	 unsigned long *set_kern_flags)
 LSM_HOOK(int, 0, move_mount, const struct path *from_path,
 	 const struct path *to_path)
-LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
+LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
 	 int mode, const struct qstr *name, const char **xattr_name,
 	 void **ctx, u32 *ctxlen)
 LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
diff --git a/security/security.c b/security/security.c
index 3d4eb474f35b..0133e60f4817 100644
--- a/security/security.c
+++ b/security/security.c
@@ -745,6 +745,19 @@ static int lsm_superblock_alloc(struct super_block *sb)
 	RC;							\
 })
 
+#define call_first_supported_int_hook(FUNC, IRC, ...) ({	\
+	int RC = IRC;						\
+	do {							\
+		struct security_hook_list *P;			\
+								\
+		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
+			RC = P->hook.FUNC(__VA_ARGS__);		\
+			if (RC != -EOPNOTSUPP)			\
+				break;				\
+		}						\
+	} while (0);						\
+	RC;							\
+})
 /* Security operations */
 
 int security_binder_set_context_mgr(const struct cred *mgr)
@@ -1048,8 +1061,9 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
 				  const char **xattr_name, void **ctx,
 				  u32 *ctxlen)
 {
-	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
-				name, xattr_name, ctx, ctxlen);
+	return call_first_supported_int_hook(dentry_init_security,
+					     -EOPNOTSUPP, dentry, mode,
+					     name, xattr_name, ctx, ctxlen);
 }
 EXPORT_SYMBOL(security_dentry_init_security);
 
-- 
2.34.1

Re: "kernel NULL pointer dereference" crash when attempting a write

[Christian Brauner]

On Tue, Jan 25, 2022 at 07:32:19AM -0500, Jeff Layton wrote:
> On Tue, 2022-01-25 at 13:12 +0100, Christian Brauner wrote:
> > On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
> > > On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
> > > > On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> > > > > On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> > > > > > On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > > > > > > 
> > > > > > > Hello,
> > > > > > > 
> > > > > > > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > > > > > > 
> > > > > > > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > > > > > > 
> > > > > > > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > > > > > > distro / arch: Arch Linux / x86_64
> > > > > > > SELinux is not enabled
> > > > > > > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > > > > > > 
> > > > > > > relevant dmesg output:
> > > > > > > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > > > > [   30.947206] #PF: supervisor read access in kernel mode
> > > > > > > [   30.947258] #PF: error_code(0x0000) - not-present page
> > > > > > > [   30.947310] PGD 0 P4D 0
> > > > > > > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > > > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > > > > > > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > > > > > > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > > > > > > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > > > > > > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > > > > > > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > > > > > > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > > > > > > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > > > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > > > > > > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > > > > > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > > > > > > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > > > > > > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > > > > > > [   30.948376] Call Trace:
> > > > > > > [   30.948404]  <TASK>
> > > > > > > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> > > > > > 
> > > > > > My guess is that the "name_len = strlen(name);" line in
> > > > > > ceph_security_init_secctx() is causing the problem as "name" is likely
> > > > > > NULL/garbage without any LSMs configured for that hook.
> > > > > > 
> > > > > > [SIDE NOTE: we should remove the comment block directly above that
> > > > > > line as it no longer applies, arguably commit 15bf32398ad4 should have
> > > > > > removed it.]
> > > > > > 
> > > > > > I'm open to suggestions on the best approach, but my gut feeling is
> > > > > > that the fix is to have the security_dentry_init_security() hook set
> > > > > > the xattr_name parameter to NULL before calling into the LSMs and then
> > > > > > the callers should check for "name == NULL" on return.  In the case of
> > > > > > ceph that would probably be something like this:
> > > > > > 
> > > > > >   name_len = (name ? strlen(name) : 0);
> > > > > > 
> > > > > > ... with fuse looking very similar, although fuse appears to need a
> > > > > > "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> > > > > > doesn't use the xattr name.
> > > > > > 
> > > > > > Thoughts?
> > > > > > 
> > > > > 
> > > > > Actually, if we don't get an xattr name back from the call, we don't
> > > > > need to do anything in ceph_security_init_secctx(). Stephen, could you
> > > > > test this patch and let us know if it fixes it?
> > > > > 
> > > > > -------------------8<--------------------
> > > > > 
> > > > > [PATCH] ceph: when no LSM is configured, don't set security xattr
> > > > > 
> > > > > If the xattr name pointer is not populated after the call to
> > > > > security_dentry_init_security(), then we should assume that no LSM is
> > > > > configured and that no xattr needs to be set for it. Also, remove a
> > > > > now-defunct comment.
> > > > > 
> > > > > Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
> > > > > Cc: Vivek Goyal <vgoyal@redhat.com>
> > > > > Reported-by: Stephen Muth <smuth4@gmail.com>
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/ceph/xattr.c | 13 +++++++------
> > > > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > > index fcf7dfdecf96..a61e1adb49a9 100644
> > > > > --- a/fs/ceph/xattr.c
> > > > > +++ b/fs/ceph/xattr.c
> > > > > @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > > >  			   struct ceph_acl_sec_ctx *as_ctx)
> > > > >  {
> > > > >  	struct ceph_pagelist *pagelist = as_ctx->pagelist;
> > > > > -	const char *name;
> > > > > +	const char *name = NULL;
> > > > >  	size_t name_len;
> > > > >  	int err;
> > > > >  
> > > > > @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > > >  		goto out;
> > > > >  	}
> > > > >  
> > > > > +	/* No LSM configured? Do nothing. */
> > > > > +	if (!name) {
> > > > 
> > > > Excuse my french but that reads very riské. Can we somehow make
> > > > security_dentry_init_security() return an error number that caller's
> > > > like ceph can use to skip?
> > > > I think that returning 0 from security_dentry_init_security() and having
> > > > *name be returned as NULL is very fragile.
> > > 
> > > Good point. In fact, I'm not sure why he didn't get an -EOPNOTSUPP
> > > return here:
> > > 
> > >         return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> > >                                 name, xattr_name, ctx, ctxlen);
> > > 
> > > It looks like the second argument is what should end up being returned
> > > when there are no entries in the security_hook_list. So it seems like
> > > maybe Stephen's security_hook_list was not empty, but didn't have a
> > > vector for dentry_init_security and returned the default 0 there?
> > 
> > Yeah, I came to the exact same analysis and subsequent question in:
> > https://lore.kernel.org/ceph-devel/20220125111111.irvr4dg3i56crqrs@wittgenstein
> > 
> > It seems to me that selinux support is compiled in in archlinux but no
> > policy loaded/enabled. I don't know if that means that the hooks is
> > called and by default returns 0 or something. I'm rather confused as the
> > call_int_hook() implies -EOPNOTSUPP is returned but looking at
> > dentry_init_security as defined in LSM_HOOKS(int, 0,
> > dentry_init_security, [...]) in include/linux/lsm_hook_defs.h implies
> > that the default return value is 0.
> 
> Yeah, the problem is the call_int_hook macro, I think, which wants to
> walk through all of the LSMs in the stack and call this for all of
> them.  What happens though if you have 2 entries with valid
> dentry_init_security hooks in them, and they both try to set the xattr
> name?
> 
> This can only ever return a single xattr_name, so maybe the right thing

Oh, I see that's similar to a bug I saw before for
security_fs_context_parse_param(). So yes, I think we need something
like what Casey sent as a reply to my analysis in:
https://lore.kernel.org/lkml/018a9bb4-accb-c19a-5b0a-fde22f4bc822@schaufler-ca.com/
for security_dentry_init_security().

(Btw, it is very odd that the bug in security_fs_context_parse_param()
still isn't fixed in master. Neither the generic lsm fix:
https://lore.kernel.org/lkml/018a9bb4-accb-c19a-5b0a-fde22f4bc822@schaufler-ca.com/
nor the fix for selinux:
https://lore.kernel.org/lkml/20211012103243.xumzerhvhklqrovj@wittgenstein/
seem to have gone anywhere? That's another NULL-deref, see:
https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943)

> to do is to make the default for the LSM_HOOK be -EOPNOTSUPP and add a
> macro that does call_int_hook, but returns after the first supported
> hook call? Something like this maybe? (untested):
> 
> -----------8<----------------
> 
> [RFC PATCH] security: dentry_init_security hook should return on first success
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  include/linux/lsm_hook_defs.h |  2 +-
>  security/security.c           | 18 ++++++++++++++++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index a5a724c308d8..819ec92dc2a8 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, const struct super_block *oldsb,
>  	 unsigned long *set_kern_flags)
>  LSM_HOOK(int, 0, move_mount, const struct path *from_path,
>  	 const struct path *to_path)
> -LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> +LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
>  	 int mode, const struct qstr *name, const char **xattr_name,
>  	 void **ctx, u32 *ctxlen)
>  LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> diff --git a/security/security.c b/security/security.c
> index 3d4eb474f35b..0133e60f4817 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -745,6 +745,19 @@ static int lsm_superblock_alloc(struct super_block *sb)
>  	RC;							\
>  })
>  
> +#define call_first_supported_int_hook(FUNC, IRC, ...) ({	\
> +	int RC = IRC;						\
> +	do {							\
> +		struct security_hook_list *P;			\
> +								\
> +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> +			RC = P->hook.FUNC(__VA_ARGS__);		\
> +			if (RC != -EOPNOTSUPP)			\
> +				break;				\
> +		}						\
> +	} while (0);						\
> +	RC;							\
> +})
>  /* Security operations */
>  
>  int security_binder_set_context_mgr(const struct cred *mgr)
> @@ -1048,8 +1061,9 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
>  				  const char **xattr_name, void **ctx,
>  				  u32 *ctxlen)
>  {
> -	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> -				name, xattr_name, ctx, ctxlen);
> +	return call_first_supported_int_hook(dentry_init_security,
> +					     -EOPNOTSUPP, dentry, mode,
> +					     name, xattr_name, ctx, ctxlen);
>  }
>  EXPORT_SYMBOL(security_dentry_init_security);
>  
> -- 
> 2.34.1
> 
> 
> 

Re: "kernel NULL pointer dereference" crash when attempting a write

[Paul Moore]

On Tue, Jan 25, 2022 at 7:49 AM Christian Brauner <brauner@kernel.org> wrote:
> (Btw, it is very odd that the bug in security_fs_context_parse_param()
> still isn't fixed in master. Neither the generic lsm fix:
> https://lore.kernel.org/lkml/018a9bb4-accb-c19a-5b0a-fde22f4bc822@schaufler-ca.com/
> nor the fix for selinux:
> https://lore.kernel.org/lkml/20211012103243.xumzerhvhklqrovj@wittgenstein/
> seem to have gone anywhere? That's another NULL-deref, see:
> https://syzkaller.appspot.com/bug?extid=d1e3b1d92d25abf97943)

Adding the LSM list to the To: line for this snippet to bring these
patches back to front of people's minds.

I suspect the issue is that these patches fall into the general LSM
"security/*.c" bin and as a result don't trigger the individual LSMs
"okay, I'll merge this behavior".  Normally I would expect this to get
picked up by James' LSM tree but sometimes the lines get blurry.

As James is my boss now I talk to him a fair amount, I'll ping him
about these patches to try and get some action on them during this -rc
cycle.  I'll also go review/tag them as well.

-- 
paul moore
paul-moore.com

Re: "kernel NULL pointer dereference" crash when attempting a write

[Vivek Goyal]

On Tue, Jan 25, 2022 at 07:32:19AM -0500, Jeff Layton wrote:
> On Tue, 2022-01-25 at 13:12 +0100, Christian Brauner wrote:
> > On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
> > > On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
> > > > On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> > > > > On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> > > > > > On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > > > > > > 
> > > > > > > Hello,
> > > > > > > 
> > > > > > > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > > > > > > 
> > > > > > > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > > > > > > 
> > > > > > > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > > > > > > distro / arch: Arch Linux / x86_64
> > > > > > > SELinux is not enabled
> > > > > > > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > > > > > > 
> > > > > > > relevant dmesg output:
> > > > > > > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > > > > [   30.947206] #PF: supervisor read access in kernel mode
> > > > > > > [   30.947258] #PF: error_code(0x0000) - not-present page
> > > > > > > [   30.947310] PGD 0 P4D 0
> > > > > > > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > > > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > > > > > > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > > > > > > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > > > > > > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > > > > > > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > > > > > > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > > > > > > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > > > > > > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > > > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > > > > > > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > > > > > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > > > > > > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > > > > > > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > > > > > > [   30.948376] Call Trace:
> > > > > > > [   30.948404]  <TASK>
> > > > > > > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> > > > > > 
> > > > > > My guess is that the "name_len = strlen(name);" line in
> > > > > > ceph_security_init_secctx() is causing the problem as "name" is likely
> > > > > > NULL/garbage without any LSMs configured for that hook.
> > > > > > 
> > > > > > [SIDE NOTE: we should remove the comment block directly above that
> > > > > > line as it no longer applies, arguably commit 15bf32398ad4 should have
> > > > > > removed it.]
> > > > > > 
> > > > > > I'm open to suggestions on the best approach, but my gut feeling is
> > > > > > that the fix is to have the security_dentry_init_security() hook set
> > > > > > the xattr_name parameter to NULL before calling into the LSMs and then
> > > > > > the callers should check for "name == NULL" on return.  In the case of
> > > > > > ceph that would probably be something like this:
> > > > > > 
> > > > > >   name_len = (name ? strlen(name) : 0);
> > > > > > 
> > > > > > ... with fuse looking very similar, although fuse appears to need a
> > > > > > "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> > > > > > doesn't use the xattr name.
> > > > > > 
> > > > > > Thoughts?
> > > > > > 
> > > > > 
> > > > > Actually, if we don't get an xattr name back from the call, we don't
> > > > > need to do anything in ceph_security_init_secctx(). Stephen, could you
> > > > > test this patch and let us know if it fixes it?
> > > > > 
> > > > > -------------------8<--------------------
> > > > > 
> > > > > [PATCH] ceph: when no LSM is configured, don't set security xattr
> > > > > 
> > > > > If the xattr name pointer is not populated after the call to
> > > > > security_dentry_init_security(), then we should assume that no LSM is
> > > > > configured and that no xattr needs to be set for it. Also, remove a
> > > > > now-defunct comment.
> > > > > 
> > > > > Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
> > > > > Cc: Vivek Goyal <vgoyal@redhat.com>
> > > > > Reported-by: Stephen Muth <smuth4@gmail.com>
> > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > ---
> > > > >  fs/ceph/xattr.c | 13 +++++++------
> > > > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > > index fcf7dfdecf96..a61e1adb49a9 100644
> > > > > --- a/fs/ceph/xattr.c
> > > > > +++ b/fs/ceph/xattr.c
> > > > > @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > > >  			   struct ceph_acl_sec_ctx *as_ctx)
> > > > >  {
> > > > >  	struct ceph_pagelist *pagelist = as_ctx->pagelist;
> > > > > -	const char *name;
> > > > > +	const char *name = NULL;
> > > > >  	size_t name_len;
> > > > >  	int err;
> > > > >  
> > > > > @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > > >  		goto out;
> > > > >  	}
> > > > >  
> > > > > +	/* No LSM configured? Do nothing. */
> > > > > +	if (!name) {
> > > > 
> > > > Excuse my french but that reads very riské. Can we somehow make
> > > > security_dentry_init_security() return an error number that caller's
> > > > like ceph can use to skip?
> > > > I think that returning 0 from security_dentry_init_security() and having
> > > > *name be returned as NULL is very fragile.
> > > 
> > > Good point. In fact, I'm not sure why he didn't get an -EOPNOTSUPP
> > > return here:
> > > 
> > >         return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> > >                                 name, xattr_name, ctx, ctxlen);
> > > 
> > > It looks like the second argument is what should end up being returned
> > > when there are no entries in the security_hook_list. So it seems like
> > > maybe Stephen's security_hook_list was not empty, but didn't have a
> > > vector for dentry_init_security and returned the default 0 there?
> > 
> > Yeah, I came to the exact same analysis and subsequent question in:
> > https://lore.kernel.org/ceph-devel/20220125111111.irvr4dg3i56crqrs@wittgenstein
> > 
> > It seems to me that selinux support is compiled in in archlinux but no
> > policy loaded/enabled. I don't know if that means that the hooks is
> > called and by default returns 0 or something. I'm rather confused as the
> > call_int_hook() implies -EOPNOTSUPP is returned but looking at
> > dentry_init_security as defined in LSM_HOOKS(int, 0,
> > dentry_init_security, [...]) in include/linux/lsm_hook_defs.h implies
> > that the default return value is 0.
> 
> Yeah, the problem is the call_int_hook macro, I think, which wants to
> walk through all of the LSMs in the stack and call this for all of
> them.  What happens though if you have 2 entries with valid
> dentry_init_security hooks in them, and they both try to set the xattr
> name?
> 
> This can only ever return a single xattr_name, so maybe the right thing
> to do is to make the default for the LSM_HOOK be -EOPNOTSUPP and add a
> macro that does call_int_hook, but returns after the first supported
> hook call? Something like this maybe? (untested):

[ cc Casey Schaufler <casey@schaufler-ca.com> ]

Actually this patch might work. IIUC, so is the problem happening due
to BPF LSM (CONFIG_BPF_LSM). I see following in bpf_lsm.c

/* For every LSM hook that allows attachment of BPF programs, declare a nop
 * function where a BPF program can be attached.
 */
#define LSM_HOOK(RET, DEFAULT, NAME, ...)       \
noinline RET bpf_lsm_##NAME(__VA_ARGS__)        \
{                                               \
        return DEFAULT;                         \
}

So if CONFIG_BPF_LSM=y, then it will automatically register a hook
for dentry_init_security() and return DEFAULT. I suspect, that what
is happening here and that's how we get return code of 0 and not
-EOPNOTSUPP.

So LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, ....) should fix
that. 

But you are right that now if both SELINUX and BPF_LSM are enabled 
and in that case we will call both the hooks and call_int_hook()
will not work anymore if BPF LSM hook is called first. It will 
return -EOPNOTSUPP without ever calling into SELinux lsm. So we
need the extra helper which continues to call into next LSM if
previous LSMs returned -EOPNOTSUPP.

This will work as long as BPF LSM always returns -EOPNOTSUPP. I don't
know enough about BPF. But can one write a BPF program and return
something other than -EOPNOTSUPP. In that case it will break.

May be not. I guess if there are multiple LSMs supporting
dentry_init_security() hooks, then intent is to call into first
LSM which does something and initializes security context. If BPF
somehow manages to do that, so it gets that opportunity and SELinux
will not be called.

Having said that, SELinux might not like the idea. 

And if SELinux is called first, then BPF hook will never be called
and any bpf program waiting to be called will be missed. I guess
this is lesser of a problem. First we need to fix ceph crash and
also make sure SELinux context is initialized (even if BPF LSM is
enabled).

Looks like dentry_init_security() can't handle multiple LSMs. We probably
should disallow all other LSMs to register a hook for this and only
allow SELinux to register a hook.

Thanks
Vivek



> 
> -----------8<----------------
> 
> [RFC PATCH] security: dentry_init_security hook should return on first success
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  include/linux/lsm_hook_defs.h |  2 +-
>  security/security.c           | 18 ++++++++++++++++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index a5a724c308d8..819ec92dc2a8 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, const struct super_block *oldsb,
>  	 unsigned long *set_kern_flags)
>  LSM_HOOK(int, 0, move_mount, const struct path *from_path,
>  	 const struct path *to_path)
> -LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> +LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
>  	 int mode, const struct qstr *name, const char **xattr_name,
>  	 void **ctx, u32 *ctxlen)
>  LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> diff --git a/security/security.c b/security/security.c
> index 3d4eb474f35b..0133e60f4817 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -745,6 +745,19 @@ static int lsm_superblock_alloc(struct super_block *sb)
>  	RC;							\
>  })
>  
> +#define call_first_supported_int_hook(FUNC, IRC, ...) ({	\
> +	int RC = IRC;						\
> +	do {							\
> +		struct security_hook_list *P;			\
> +								\
> +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> +			RC = P->hook.FUNC(__VA_ARGS__);		\
> +			if (RC != -EOPNOTSUPP)			\
> +				break;				\
> +		}						\
> +	} while (0);						\
> +	RC;							\
> +})
>  /* Security operations */
>  
>  int security_binder_set_context_mgr(const struct cred *mgr)
> @@ -1048,8 +1061,9 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
>  				  const char **xattr_name, void **ctx,
>  				  u32 *ctxlen)
>  {
> -	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> -				name, xattr_name, ctx, ctxlen);
> +	return call_first_supported_int_hook(dentry_init_security,
> +					     -EOPNOTSUPP, dentry, mode,
> +					     name, xattr_name, ctx, ctxlen);
>  }
>  EXPORT_SYMBOL(security_dentry_init_security);
>  
> -- 
> 2.34.1
> 
> 
> 

Re: "kernel NULL pointer dereference" crash when attempting a write

[Casey Schaufler]

On 1/25/2022 7:56 AM, Vivek Goyal wrote:
> On Tue, Jan 25, 2022 at 07:32:19AM -0500, Jeff Layton wrote:
>> On Tue, 2022-01-25 at 13:12 +0100, Christian Brauner wrote:
>>> On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
>>>> On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
>>>>> On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
>>>>>> On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
>>>>>>> On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
>>>>>>>>
>>>>>>>> This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
>>>>>>>>
>>>>>>>> /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
>>>>>>>> distro / arch: Arch Linux / x86_64
>>>>>>>> SELinux is not enabled
>>>>>>>> ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
>>>>>>>>
>>>>>>>> relevant dmesg output:
>>>>>>>> [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
>>>>>>>> [   30.947206] #PF: supervisor read access in kernel mode
>>>>>>>> [   30.947258] #PF: error_code(0x0000) - not-present page
>>>>>>>> [   30.947310] PGD 0 P4D 0
>>>>>>>> [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
>>>>>>>> [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
>>>>>>>> [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
>>>>>>>> [   30.947569] RIP: 0010:strlen+0x0/0x20
>>>>>>>> [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
>>>>>>>> f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
>>>>>>>> [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
>>>>>>>> [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
>>>>>>>> [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>>>>>>>> [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
>>>>>>>> [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
>>>>>>>> [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
>>>>>>>> [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
>>>>>>>> [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>>>>>> [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
>>>>>>>> [   30.948376] Call Trace:
>>>>>>>> [   30.948404]  <TASK>
>>>>>>>> [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
>>>>>>> My guess is that the "name_len = strlen(name);" line in
>>>>>>> ceph_security_init_secctx() is causing the problem as "name" is likely
>>>>>>> NULL/garbage without any LSMs configured for that hook.
>>>>>>>
>>>>>>> [SIDE NOTE: we should remove the comment block directly above that
>>>>>>> line as it no longer applies, arguably commit 15bf32398ad4 should have
>>>>>>> removed it.]
>>>>>>>
>>>>>>> I'm open to suggestions on the best approach, but my gut feeling is
>>>>>>> that the fix is to have the security_dentry_init_security() hook set
>>>>>>> the xattr_name parameter to NULL before calling into the LSMs and then
>>>>>>> the callers should check for "name == NULL" on return.  In the case of
>>>>>>> ceph that would probably be something like this:
>>>>>>>
>>>>>>>    name_len = (name ? strlen(name) : 0);
>>>>>>>
>>>>>>> ... with fuse looking very similar, although fuse appears to need a
>>>>>>> "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
>>>>>>> doesn't use the xattr name.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>> Actually, if we don't get an xattr name back from the call, we don't
>>>>>> need to do anything in ceph_security_init_secctx(). Stephen, could you
>>>>>> test this patch and let us know if it fixes it?
>>>>>>
>>>>>> -------------------8<--------------------
>>>>>>
>>>>>> [PATCH] ceph: when no LSM is configured, don't set security xattr
>>>>>>
>>>>>> If the xattr name pointer is not populated after the call to
>>>>>> security_dentry_init_security(), then we should assume that no LSM is
>>>>>> configured and that no xattr needs to be set for it. Also, remove a
>>>>>> now-defunct comment.
>>>>>>
>>>>>> Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
>>>>>> Cc: Vivek Goyal <vgoyal@redhat.com>
>>>>>> Reported-by: Stephen Muth <smuth4@gmail.com>
>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>>> ---
>>>>>>   fs/ceph/xattr.c | 13 +++++++------
>>>>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
>>>>>> index fcf7dfdecf96..a61e1adb49a9 100644
>>>>>> --- a/fs/ceph/xattr.c
>>>>>> +++ b/fs/ceph/xattr.c
>>>>>> @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
>>>>>>   			   struct ceph_acl_sec_ctx *as_ctx)
>>>>>>   {
>>>>>>   	struct ceph_pagelist *pagelist = as_ctx->pagelist;
>>>>>> -	const char *name;
>>>>>> +	const char *name = NULL;
>>>>>>   	size_t name_len;
>>>>>>   	int err;
>>>>>>   
>>>>>> @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
>>>>>>   		goto out;
>>>>>>   	}
>>>>>>   
>>>>>> +	/* No LSM configured? Do nothing. */
>>>>>> +	if (!name) {
>>>>> Excuse my french but that reads very riské. Can we somehow make
>>>>> security_dentry_init_security() return an error number that caller's
>>>>> like ceph can use to skip?
>>>>> I think that returning 0 from security_dentry_init_security() and having
>>>>> *name be returned as NULL is very fragile.
>>>> Good point. In fact, I'm not sure why he didn't get an -EOPNOTSUPP
>>>> return here:
>>>>
>>>>          return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>>>>                                  name, xattr_name, ctx, ctxlen);
>>>>
>>>> It looks like the second argument is what should end up being returned
>>>> when there are no entries in the security_hook_list. So it seems like
>>>> maybe Stephen's security_hook_list was not empty, but didn't have a
>>>> vector for dentry_init_security and returned the default 0 there?
>>> Yeah, I came to the exact same analysis and subsequent question in:
>>> https://lore.kernel.org/ceph-devel/20220125111111.irvr4dg3i56crqrs@wittgenstein
>>>
>>> It seems to me that selinux support is compiled in in archlinux but no
>>> policy loaded/enabled. I don't know if that means that the hooks is
>>> called and by default returns 0 or something. I'm rather confused as the
>>> call_int_hook() implies -EOPNOTSUPP is returned but looking at
>>> dentry_init_security as defined in LSM_HOOKS(int, 0,
>>> dentry_init_security, [...]) in include/linux/lsm_hook_defs.h implies
>>> that the default return value is 0.
>> Yeah, the problem is the call_int_hook macro, I think, which wants to
>> walk through all of the LSMs in the stack and call this for all of
>> them.  What happens though if you have 2 entries with valid
>> dentry_init_security hooks in them, and they both try to set the xattr
>> name?
>>
>> This can only ever return a single xattr_name, so maybe the right thing
>> to do is to make the default for the LSM_HOOK be -EOPNOTSUPP and add a
>> macro that does call_int_hook, but returns after the first supported
>> hook call? Something like this maybe? (untested):
> [ cc Casey Schaufler <casey@schaufler-ca.com> ]

Joining the conversation late. Wish someone had brought me
in sooner.

> Actually this patch might work. IIUC, so is the problem happening due
> to BPF LSM (CONFIG_BPF_LSM). I see following in bpf_lsm.c
>
> /* For every LSM hook that allows attachment of BPF programs, declare a nop
>   * function where a BPF program can be attached.
>   */
> #define LSM_HOOK(RET, DEFAULT, NAME, ...)       \
> noinline RET bpf_lsm_##NAME(__VA_ARGS__)        \
> {                                               \
>          return DEFAULT;                         \
> }
>
> So if CONFIG_BPF_LSM=y, then it will automatically register a hook
> for dentry_init_security() and return DEFAULT. I suspect, that what
> is happening here and that's how we get return code of 0 and not
> -EOPNOTSUPP.
>
> So LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, ....) should fix
> that.
>
> But you are right that now if both SELINUX and BPF_LSM are enabled
> and in that case we will call both the hooks and call_int_hook()
> will not work anymore if BPF LSM hook is called first. It will
> return -EOPNOTSUPP without ever calling into SELinux lsm. So we
> need the extra helper which continues to call into next LSM if
> previous LSMs returned -EOPNOTSUPP.

This is a case where the LSM hook code needs to eschew call_int_hook()
and open code the required logic. Look at security_inode_getsecurity().
Please resist the temptation to add more macros.

>
> This will work as long as BPF LSM always returns -EOPNOTSUPP. I don't
> know enough about BPF. But can one write a BPF program and return
> something other than -EOPNOTSUPP. In that case it will break.
>
> May be not. I guess if there are multiple LSMs supporting
> dentry_init_security() hooks, then intent is to call into first
> LSM which does something and initializes security context. If BPF
> somehow manages to do that, so it gets that opportunity and SELinux
> will not be called.
>
> Having said that, SELinux might not like the idea.

BPF provides all LSM hooks, all the time. So long as BPF
is stacked last you should be OK. The BPF developers claim
that it has to be this way.

>
> And if SELinux is called first, then BPF hook will never be called
> and any bpf program waiting to be called will be missed. I guess
> this is lesser of a problem. First we need to fix ceph crash and
> also make sure SELinux context is initialized (even if BPF LSM is
> enabled).
>
> Looks like dentry_init_security() can't handle multiple LSMs. We probably
> should disallow all other LSMs to register a hook for this and only
> allow SELinux to register a hook.

Not acceptable. The fix to dentry_init_security() is easy.

>
> Thanks
> Vivek
>
>
>
>> -----------8<----------------
>>
>> [RFC PATCH] security: dentry_init_security hook should return on first success
>>
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>   include/linux/lsm_hook_defs.h |  2 +-
>>   security/security.c           | 18 ++++++++++++++++--
>>   2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index a5a724c308d8..819ec92dc2a8 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, const struct super_block *oldsb,
>>   	 unsigned long *set_kern_flags)
>>   LSM_HOOK(int, 0, move_mount, const struct path *from_path,
>>   	 const struct path *to_path)
>> -LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
>> +LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
>>   	 int mode, const struct qstr *name, const char **xattr_name,
>>   	 void **ctx, u32 *ctxlen)
>>   LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
>> diff --git a/security/security.c b/security/security.c
>> index 3d4eb474f35b..0133e60f4817 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -745,6 +745,19 @@ static int lsm_superblock_alloc(struct super_block *sb)
>>   	RC;							\
>>   })
>>   
>> +#define call_first_supported_int_hook(FUNC, IRC, ...) ({	\
>> +	int RC = IRC;						\
>> +	do {							\
>> +		struct security_hook_list *P;			\
>> +								\
>> +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
>> +			RC = P->hook.FUNC(__VA_ARGS__);		\
>> +			if (RC != -EOPNOTSUPP)			\
>> +				break;				\
>> +		}						\
>> +	} while (0);						\
>> +	RC;							\
>> +})
>>   /* Security operations */
>>   
>>   int security_binder_set_context_mgr(const struct cred *mgr)
>> @@ -1048,8 +1061,9 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
>>   				  const char **xattr_name, void **ctx,
>>   				  u32 *ctxlen)
>>   {
>> -	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>> -				name, xattr_name, ctx, ctxlen);
>> +	return call_first_supported_int_hook(dentry_init_security,
>> +					     -EOPNOTSUPP, dentry, mode,
>> +					     name, xattr_name, ctx, ctxlen);
>>   }
>>   EXPORT_SYMBOL(security_dentry_init_security);
>>   
>> -- 
>> 2.34.1
>>
>>
>>

Re: "kernel NULL pointer dereference" crash when attempting a write

[Paul Moore]

On Tue, Jan 25, 2022 at 12:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 1/25/2022 7:56 AM, Vivek Goyal wrote:
> > On Tue, Jan 25, 2022 at 07:32:19AM -0500, Jeff Layton wrote:
> >> On Tue, 2022-01-25 at 13:12 +0100, Christian Brauner wrote:
> >>> On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
> >>>> On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
> >>>>> On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> >>>>>> On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> >>>>>>> On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:

...

> Joining the conversation late. Wish someone had brought me
> in sooner.

For some reason I thought the LSM list was on the To/CC line, my
mistake (fixed now).

Thanks to everyone for all of the further discussion, review on this;
I plucked the original post out of my spam folder as I was shutting
down for the night yesterday and only gave it a quick look.

> > Looks like dentry_init_security() can't handle multiple LSMs. We probably
> > should disallow all other LSMs to register a hook for this and only
> > allow SELinux to register a hook.
>
> Not acceptable. The fix to dentry_init_security() is easy.

Sounds good to me, Vivek did you want to put together a patch for
this?  If not, let me know and I'll put one together.

-- 
paul-moore.com

Re: "kernel NULL pointer dereference" crash when attempting a write

[Casey Schaufler]

On 1/25/2022 11:57 AM, Paul Moore wrote:
> On Tue, Jan 25, 2022 at 12:09 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 1/25/2022 7:56 AM, Vivek Goyal wrote:
>>> On Tue, Jan 25, 2022 at 07:32:19AM -0500, Jeff Layton wrote:
>>>> On Tue, 2022-01-25 at 13:12 +0100, Christian Brauner wrote:
>>>>> On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
>>>>>> On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
>>>>>>> On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
>>>>>>>> On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
>>>>>>>>> On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> ...
>
>> Joining the conversation late. Wish someone had brought me
>> in sooner.
> For some reason I thought the LSM list was on the To/CC line, my
> mistake (fixed now).
>
> Thanks to everyone for all of the further discussion, review on this;
> I plucked the original post out of my spam folder as I was shutting
> down for the night yesterday and only gave it a quick look.
>
>>> Looks like dentry_init_security() can't handle multiple LSMs. We probably
>>> should disallow all other LSMs to register a hook for this and only
>>> allow SELinux to register a hook.
>> Not acceptable. The fix to dentry_init_security() is easy.
> Sounds good to me, Vivek did you want to put together a patch for
> this?  If not, let me know and I'll put one together.

Thank you. I'd do it myself, but I'm dealing with pain meds and a
hinky keyboard. Rarely a great start for a good patch.

Re: "kernel NULL pointer dereference" crash when attempting a write

[Vivek Goyal]

On Tue, Jan 25, 2022 at 02:57:46PM -0500, Paul Moore wrote:
> 
> > > Looks like dentry_init_security() can't handle multiple LSMs. We probably
> > > should disallow all other LSMs to register a hook for this and only
> > > allow SELinux to register a hook.
> >
> > Not acceptable. The fix to dentry_init_security() is easy.
> 
> Sounds good to me, Vivek did you want to put together a patch for
> this?  If not, let me know and I'll put one together.

Ok, I have put together this test patch. Stephen Muth, can you please
test it and let us know if it solves your problem.

I enabled CONFIG_BPF_LSM=y but that itself does not seem to be sufficient
for BPF to register a hook for dentry_init_security. So I don't see
it being called in my testing. IOW, I have not been able to reproduce
the issue and will rely on testing from Stephen to know if it it indeed
solved the problem for him or not.

-------------------8<--------------------

Subject: lsm: dentry_init_security(): Deal with multiple LSMs registering hook

A ceph user has reported that ceph is crashing with kernel NULL pointer
dereference. Following is backtrace.

/proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC)
11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022
16:18:29 +0000
distro / arch: Arch Linux / x86_64
SELinux is not enabled
ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)

relevant dmesg output:
[   30.947129] BUG: kernel NULL pointer dereference, address:
0000000000000000
[   30.947206] #PF: supervisor read access in kernel mode
[   30.947258] #PF: error_code(0x0000) - not-present page
[   30.947310] PGD 0 P4D 0
[   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
[   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1
86fbf2c313cc37a553d65deb81d98e9dcc2a3659
[   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M
DS3H/B365M DS3H, BIOS F5 08/13/2019
[   30.947569] RIP: 0010:strlen+0x0/0x20
[   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75
ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31
ff
[   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
[   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX:
0000000000000000
[   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
0000000000000000
[   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09:
0000000000000000
[   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12:
0000000000000000
[   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15:
0000000000000000
[   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000)
knlGS:0000000000000000
[   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4:
00000000003706e0
[   30.948376] Call Trace:
[   30.948404]  <TASK>
[   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph
49f9c4b9bf5be8760f19f1747e26da33920bce4b]
[   30.948582]  ceph_atomic_open+0x51e/0x8a0 [ceph
49f9c4b9bf5be8760f19f1747e26da33920bce4b]
[   30.948708]  ? get_cached_acl+0x4d/0xa0
[   30.948759]  path_openat+0x60d/0x1030
[   30.948809]  do_filp_open+0xa5/0x150
[   30.948859]  do_sys_openat2+0xc4/0x190
[   30.948904]  __x64_sys_openat+0x53/0xa0
[   30.948948]  do_syscall_64+0x5c/0x90
[   30.948989]  ? exc_page_fault+0x72/0x180
[   30.949034]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   30.949091] RIP: 0033:0x7fc7521e25bb
[   30.950849] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00
00 00 85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 0
0 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 91 00 00 00 48 8b 54 24 28 64 48 2b 14
25

Core of the problem is that ceph checks for return code from
security_dentry_init_security() and if 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 ceph deals with it.

But somehow in this configuration, 0 is being returned and "name" is
not being initialized, and that's creating the problem.

Our suspicion is that BPF LSM is registering a hook for
dentry_init_security() and returns hook default of 0. I have not been
able to configure it that way so I am not 100% sure.

LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,...)

dentry_init_security() is written in such a way that it expects only one
LSM to register the hook. Atleast that's the expectation with current code.

If another LSM returns a hook and returns default, it will simply return
0 as of now and that will break ceph. 

Anyway, suggestion is that change semantics of this hook a bit. If there
are no LSMs or no LSM is taking ownership and initializing security context,
then return -EOPNOTSUP. Also allow at max one LSM to initialize security
context. This hook can't deal with multiple LSMs trying to init security
context. This patch implements this new behavior.

Reported-by: Stephen Muth <smuth4@gmail.com>
Suggested-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: Jeff Layton <jlayton@kernel.org>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Paul Moore <paul@paul-moore.com>
Yet-to-by-Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/linux/lsm_hook_defs.h |    2 +-
 security/security.c           |   15 +++++++++++++--
 2 files changed, 14 insertions(+), 3 deletions(-)

Index: redhat-linux/include/linux/lsm_hook_defs.h
===================================================================
--- redhat-linux.orig/include/linux/lsm_hook_defs.h	2022-01-24 14:56:14.338030140 -0500
+++ redhat-linux/include/linux/lsm_hook_defs.h	2022-01-25 18:48:46.917496696 -0500
@@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, cons
 	 unsigned long *set_kern_flags)
 LSM_HOOK(int, 0, move_mount, const struct path *from_path,
 	 const struct path *to_path)
-LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
+LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
 	 int mode, const struct qstr *name, const char **xattr_name,
 	 void **ctx, u32 *ctxlen)
 LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
Index: redhat-linux/security/security.c
===================================================================
--- redhat-linux.orig/security/security.c	2022-01-25 18:46:59.166496696 -0500
+++ redhat-linux/security/security.c	2022-01-25 18:56:25.251496696 -0500
@@ -1048,8 +1048,19 @@ int security_dentry_init_security(struct
 				  const char **xattr_name, void **ctx,
 				  u32 *ctxlen)
 {
-	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
-				name, xattr_name, ctx, ctxlen);
+	struct security_hook_list *hp;
+	int rc;
+
+	/*
+	 * Only one module will provide a security context.
+	 */
+	hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) {
+		rc = hp->hook.dentry_init_security(dentry, mode, name,
+						   xattr_name, ctx, ctxlen);
+		if (rc != LSM_RET_DEFAULT(dentry_init_security))
+			return rc;
+	}
+	return LSM_RET_DEFAULT(dentry_init_security);
 }
 EXPORT_SYMBOL(security_dentry_init_security);
 

Re: "kernel NULL pointer dereference" crash when attempting a write

[Casey Schaufler]

On 1/25/2022 4:27 PM, Vivek Goyal wrote:
> On Tue, Jan 25, 2022 at 02:57:46PM -0500, Paul Moore wrote:
>>>> Looks like dentry_init_security() can't handle multiple LSMs. We probably
>>>> should disallow all other LSMs to register a hook for this and only
>>>> allow SELinux to register a hook.
>>> Not acceptable. The fix to dentry_init_security() is easy.
>> Sounds good to me, Vivek did you want to put together a patch for
>> this?  If not, let me know and I'll put one together.
> Ok, I have put together this test patch. Stephen Muth, can you please
> test it and let us know if it solves your problem.
>
> I enabled CONFIG_BPF_LSM=y but that itself does not seem to be sufficient
> for BPF to register a hook for dentry_init_security. So I don't see
> it being called in my testing. IOW, I have not been able to reproduce
> the issue and will rely on testing from Stephen to know if it it indeed
> solved the problem for him or not.
>
> -------------------8<--------------------
>
> Subject: lsm: dentry_init_security(): Deal with multiple LSMs registering hook
>
> A ceph user has reported that ceph is crashing with kernel NULL pointer
> dereference. Following is backtrace.
>
> /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC)
> 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022
> 16:18:29 +0000
> distro / arch: Arch Linux / x86_64
> SELinux is not enabled
> ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
>
> relevant dmesg output:
> [   30.947129] BUG: kernel NULL pointer dereference, address:
> 0000000000000000
> [   30.947206] #PF: supervisor read access in kernel mode
> [   30.947258] #PF: error_code(0x0000) - not-present page
> [   30.947310] PGD 0 P4D 0
> [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1
> 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M
> DS3H/B365M DS3H, BIOS F5 08/13/2019
> [   30.947569] RIP: 0010:strlen+0x0/0x20
> [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75
> ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31
> ff
> [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX:
> 0000000000000000
> [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> 0000000000000000
> [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09:
> 0000000000000000
> [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12:
> 0000000000000000
> [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15:
> 0000000000000000
> [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000)
> knlGS:0000000000000000
> [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4:
> 00000000003706e0
> [   30.948376] Call Trace:
> [   30.948404]  <TASK>
> [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph
> 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> [   30.948582]  ceph_atomic_open+0x51e/0x8a0 [ceph
> 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> [   30.948708]  ? get_cached_acl+0x4d/0xa0
> [   30.948759]  path_openat+0x60d/0x1030
> [   30.948809]  do_filp_open+0xa5/0x150
> [   30.948859]  do_sys_openat2+0xc4/0x190
> [   30.948904]  __x64_sys_openat+0x53/0xa0
> [   30.948948]  do_syscall_64+0x5c/0x90
> [   30.948989]  ? exc_page_fault+0x72/0x180
> [   30.949034]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [   30.949091] RIP: 0033:0x7fc7521e25bb
> [   30.950849] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00
> 00 00 85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 0
> 0 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 91 00 00 00 48 8b 54 24 28 64 48 2b 14
> 25
>
> Core of the problem is that ceph checks for return code from
> security_dentry_init_security() and if 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 ceph deals with it.
>
> But somehow in this configuration, 0 is being returned and "name" is
> not being initialized, and that's creating the problem.
>
> Our suspicion is that BPF LSM is registering a hook for
> dentry_init_security() and returns hook default of 0. I have not been
> able to configure it that way so I am not 100% sure.
>
> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,...)
>
> dentry_init_security() is written in such a way that it expects only one
> LSM to register the hook. Atleast that's the expectation with current code.
>
> If another LSM returns a hook and returns default, it will simply return
> 0 as of now and that will break ceph.
>
> Anyway, suggestion is that change semantics of this hook a bit. If there
> are no LSMs or no LSM is taking ownership and initializing security context,
> then return -EOPNOTSUP. Also allow at max one LSM to initialize security
> context. This hook can't deal with multiple LSMs trying to init security
> context. This patch implements this new behavior.
>
> Reported-by: Stephen Muth <smuth4@gmail.com>
> Suggested-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Yet-to-by-Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Acked-by: Casey Schaufler <casey@schaufler-ca.com>

There are way too many places where filesystems and networking
assume 1) that there's at most one LSM and 2) that that LSM
is SELinux. BPF breaks that completely. We've been lucky in that
no one is using BPF in all the ways it could be. I'm trying
to get through all these issues, but sometimes y'all are just
moving faster than I can keep ahead of.

> ---
>   include/linux/lsm_hook_defs.h |    2 +-
>   security/security.c           |   15 +++++++++++++--
>   2 files changed, 14 insertions(+), 3 deletions(-)
>
> Index: redhat-linux/include/linux/lsm_hook_defs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/lsm_hook_defs.h	2022-01-24 14:56:14.338030140 -0500
> +++ redhat-linux/include/linux/lsm_hook_defs.h	2022-01-25 18:48:46.917496696 -0500
> @@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, cons
>   	 unsigned long *set_kern_flags)
>   LSM_HOOK(int, 0, move_mount, const struct path *from_path,
>   	 const struct path *to_path)
> -LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> +LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
>   	 int mode, const struct qstr *name, const char **xattr_name,
>   	 void **ctx, u32 *ctxlen)
>   LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> Index: redhat-linux/security/security.c
> ===================================================================
> --- redhat-linux.orig/security/security.c	2022-01-25 18:46:59.166496696 -0500
> +++ redhat-linux/security/security.c	2022-01-25 18:56:25.251496696 -0500
> @@ -1048,8 +1048,19 @@ int security_dentry_init_security(struct
>   				  const char **xattr_name, void **ctx,
>   				  u32 *ctxlen)
>   {
> -	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> -				name, xattr_name, ctx, ctxlen);
> +	struct security_hook_list *hp;
> +	int rc;
> +
> +	/*
> +	 * Only one module will provide a security context.
> +	 */
> +	hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) {
> +		rc = hp->hook.dentry_init_security(dentry, mode, name,
> +						   xattr_name, ctx, ctxlen);
> +		if (rc != LSM_RET_DEFAULT(dentry_init_security))
> +			return rc;
> +	}
> +	return LSM_RET_DEFAULT(dentry_init_security);
>   }
>   EXPORT_SYMBOL(security_dentry_init_security);
>   
>

Re: "kernel NULL pointer dereference" crash when attempting a write

[Stephen Muth]

On Tue, Jan 25, 2022 at 7:27 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jan 25, 2022 at 02:57:46PM -0500, Paul Moore wrote:
> >
> > > > Looks like dentry_init_security() can't handle multiple LSMs. We probably
> > > > should disallow all other LSMs to register a hook for this and only
> > > > allow SELinux to register a hook.
> > >
> > > Not acceptable. The fix to dentry_init_security() is easy.
> >
> > Sounds good to me, Vivek did you want to put together a patch for
> > this?  If not, let me know and I'll put one together.
>
> Ok, I have put together this test patch. Stephen Muth, can you please
> test it and let us know if it solves your problem.
>
Just tested it, and had no problem making writes.
(Apologies for any double sends, accidentally had rich text enabled
the first time)

Re: "kernel NULL pointer dereference" crash when attempting a write

[Serge E. Hallyn]

On Tue, Jan 25, 2022 at 07:27:38PM -0500, Vivek Goyal wrote:
> On Tue, Jan 25, 2022 at 02:57:46PM -0500, Paul Moore wrote:
> > 
> > > > Looks like dentry_init_security() can't handle multiple LSMs. We probably
> > > > should disallow all other LSMs to register a hook for this and only
> > > > allow SELinux to register a hook.
> > >
> > > Not acceptable. The fix to dentry_init_security() is easy.
> > 
> > Sounds good to me, Vivek did you want to put together a patch for
> > this?  If not, let me know and I'll put one together.
> 
> Ok, I have put together this test patch. Stephen Muth, can you please
> test it and let us know if it solves your problem.
> 
> I enabled CONFIG_BPF_LSM=y but that itself does not seem to be sufficient
> for BPF to register a hook for dentry_init_security. So I don't see
> it being called in my testing. IOW, I have not been able to reproduce
> the issue and will rely on testing from Stephen to know if it it indeed
> solved the problem for him or not.
> 
> -------------------8<--------------------
> 
> Subject: lsm: dentry_init_security(): Deal with multiple LSMs registering hook
> 
> A ceph user has reported that ceph is crashing with kernel NULL pointer
> dereference. Following is backtrace.
> 
> /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC)
> 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022
> 16:18:29 +0000
> distro / arch: Arch Linux / x86_64
> SELinux is not enabled
> ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> 
> relevant dmesg output:
> [   30.947129] BUG: kernel NULL pointer dereference, address:
> 0000000000000000
> [   30.947206] #PF: supervisor read access in kernel mode
> [   30.947258] #PF: error_code(0x0000) - not-present page
> [   30.947310] PGD 0 P4D 0
> [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1
> 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M
> DS3H/B365M DS3H, BIOS F5 08/13/2019
> [   30.947569] RIP: 0010:strlen+0x0/0x20
> [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75
> ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31
> ff
> [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX:
> 0000000000000000
> [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> 0000000000000000
> [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09:
> 0000000000000000
> [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12:
> 0000000000000000
> [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15:
> 0000000000000000
> [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000)
> knlGS:0000000000000000
> [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4:
> 00000000003706e0
> [   30.948376] Call Trace:
> [   30.948404]  <TASK>
> [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph
> 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> [   30.948582]  ceph_atomic_open+0x51e/0x8a0 [ceph
> 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> [   30.948708]  ? get_cached_acl+0x4d/0xa0
> [   30.948759]  path_openat+0x60d/0x1030
> [   30.948809]  do_filp_open+0xa5/0x150
> [   30.948859]  do_sys_openat2+0xc4/0x190
> [   30.948904]  __x64_sys_openat+0x53/0xa0
> [   30.948948]  do_syscall_64+0x5c/0x90
> [   30.948989]  ? exc_page_fault+0x72/0x180
> [   30.949034]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> [   30.949091] RIP: 0033:0x7fc7521e25bb
> [   30.950849] Code: 25 00 00 41 00 3d 00 00 41 00 74 4b 64 8b 04 25 18 00
> 00 00 85 c0 75 67 44 89 e2 48 89 ee bf 9c ff ff ff b8 01 01 0
> 0 00 0f 05 <48> 3d 00 f0 ff ff 0f 87 91 00 00 00 48 8b 54 24 28 64 48 2b 14
> 25
> 
> Core of the problem is that ceph checks for return code from
> security_dentry_init_security() and if 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 ceph deals with it.
> 
> But somehow in this configuration, 0 is being returned and "name" is
> not being initialized, and that's creating the problem.
> 
> Our suspicion is that BPF LSM is registering a hook for
> dentry_init_security() and returns hook default of 0. I have not been
> able to configure it that way so I am not 100% sure.
> 
> LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,...)
> 
> dentry_init_security() is written in such a way that it expects only one
> LSM to register the hook. Atleast that's the expectation with current code.
> 
> If another LSM returns a hook and returns default, it will simply return
> 0 as of now and that will break ceph. 
> 
> Anyway, suggestion is that change semantics of this hook a bit. If there
> are no LSMs or no LSM is taking ownership and initializing security context,
> then return -EOPNOTSUP. Also allow at max one LSM to initialize security
> context. This hook can't deal with multiple LSMs trying to init security
> context. This patch implements this new behavior.
> 
> Reported-by: Stephen Muth <smuth4@gmail.com>
> Suggested-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: Jeff Layton <jlayton@kernel.org>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Paul Moore <paul@paul-moore.com>
> Yet-to-by-Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Serge Hallyn <serge@hallyn.com>

Seems good, and indeed we can't have >1 such LSMS writing to the
xattr.  Thanks.

> ---
>  include/linux/lsm_hook_defs.h |    2 +-
>  security/security.c           |   15 +++++++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> Index: redhat-linux/include/linux/lsm_hook_defs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/lsm_hook_defs.h	2022-01-24 14:56:14.338030140 -0500
> +++ redhat-linux/include/linux/lsm_hook_defs.h	2022-01-25 18:48:46.917496696 -0500
> @@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, cons
>  	 unsigned long *set_kern_flags)
>  LSM_HOOK(int, 0, move_mount, const struct path *from_path,
>  	 const struct path *to_path)
> -LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> +LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
>  	 int mode, const struct qstr *name, const char **xattr_name,
>  	 void **ctx, u32 *ctxlen)
>  LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> Index: redhat-linux/security/security.c
> ===================================================================
> --- redhat-linux.orig/security/security.c	2022-01-25 18:46:59.166496696 -0500
> +++ redhat-linux/security/security.c	2022-01-25 18:56:25.251496696 -0500
> @@ -1048,8 +1048,19 @@ int security_dentry_init_security(struct
>  				  const char **xattr_name, void **ctx,
>  				  u32 *ctxlen)
>  {
> -	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> -				name, xattr_name, ctx, ctxlen);
> +	struct security_hook_list *hp;
> +	int rc;
> +
> +	/*
> +	 * Only one module will provide a security context.
> +	 */
> +	hlist_for_each_entry(hp, &security_hook_heads.dentry_init_security, list) {
> +		rc = hp->hook.dentry_init_security(dentry, mode, name,
> +						   xattr_name, ctx, ctxlen);
> +		if (rc != LSM_RET_DEFAULT(dentry_init_security))
> +			return rc;
> +	}
> +	return LSM_RET_DEFAULT(dentry_init_security);
>  }
>  EXPORT_SYMBOL(security_dentry_init_security);
>  

Re: "kernel NULL pointer dereference" crash when attempting a write

[Vivek Goyal]

On Tue, Jan 25, 2022 at 09:08:59AM -0800, Casey Schaufler wrote:
> On 1/25/2022 7:56 AM, Vivek Goyal wrote:
> > On Tue, Jan 25, 2022 at 07:32:19AM -0500, Jeff Layton wrote:
> > > On Tue, 2022-01-25 at 13:12 +0100, Christian Brauner wrote:
> > > > On Tue, Jan 25, 2022 at 06:25:39AM -0500, Jeff Layton wrote:
> > > > > On Tue, 2022-01-25 at 12:13 +0100, Christian Brauner wrote:
> > > > > > On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> > > > > > > On Mon, 2022-01-24 at 21:45 -0500, Paul Moore wrote:
> > > > > > > > On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> > > > > > > > > Hello,
> > > > > > > > > 
> > > > > > > > > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> > > > > > > > > 
> > > > > > > > > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> > > > > > > > > 
> > > > > > > > > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > > > > > > > > distro / arch: Arch Linux / x86_64
> > > > > > > > > SELinux is not enabled
> > > > > > > > > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> > > > > > > > > 
> > > > > > > > > relevant dmesg output:
> > > > > > > > > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > > > > > > [   30.947206] #PF: supervisor read access in kernel mode
> > > > > > > > > [   30.947258] #PF: error_code(0x0000) - not-present page
> > > > > > > > > [   30.947310] PGD 0 P4D 0
> > > > > > > > > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > > > > > > > > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > > > > > > > > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > > > > > > > > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > > > > > > > > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > > > > > > > > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > > > > > > > > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > > > > > > > > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > > > > > > > > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > > > > > > > > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > > > > > > > > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > > > > > > > > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > > > > > > > > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > > > > > > > > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > > > > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > > > > > > > > [   30.948376] Call Trace:
> > > > > > > > > [   30.948404]  <TASK>
> > > > > > > > > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> > > > > > > > My guess is that the "name_len = strlen(name);" line in
> > > > > > > > ceph_security_init_secctx() is causing the problem as "name" is likely
> > > > > > > > NULL/garbage without any LSMs configured for that hook.
> > > > > > > > 
> > > > > > > > [SIDE NOTE: we should remove the comment block directly above that
> > > > > > > > line as it no longer applies, arguably commit 15bf32398ad4 should have
> > > > > > > > removed it.]
> > > > > > > > 
> > > > > > > > I'm open to suggestions on the best approach, but my gut feeling is
> > > > > > > > that the fix is to have the security_dentry_init_security() hook set
> > > > > > > > the xattr_name parameter to NULL before calling into the LSMs and then
> > > > > > > > the callers should check for "name == NULL" on return.  In the case of
> > > > > > > > ceph that would probably be something like this:
> > > > > > > > 
> > > > > > > >    name_len = (name ? strlen(name) : 0);
> > > > > > > > 
> > > > > > > > ... with fuse looking very similar, although fuse appears to need a
> > > > > > > > "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> > > > > > > > doesn't use the xattr name.
> > > > > > > > 
> > > > > > > > Thoughts?
> > > > > > > > 
> > > > > > > Actually, if we don't get an xattr name back from the call, we don't
> > > > > > > need to do anything in ceph_security_init_secctx(). Stephen, could you
> > > > > > > test this patch and let us know if it fixes it?
> > > > > > > 
> > > > > > > -------------------8<--------------------
> > > > > > > 
> > > > > > > [PATCH] ceph: when no LSM is configured, don't set security xattr
> > > > > > > 
> > > > > > > If the xattr name pointer is not populated after the call to
> > > > > > > security_dentry_init_security(), then we should assume that no LSM is
> > > > > > > configured and that no xattr needs to be set for it. Also, remove a
> > > > > > > now-defunct comment.
> > > > > > > 
> > > > > > > Fixes: 15bf32398ad4 ("security: Return xattr name from security_dentry_init_security()")
> > > > > > > Cc: Vivek Goyal <vgoyal@redhat.com>
> > > > > > > Reported-by: Stephen Muth <smuth4@gmail.com>
> > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > > > ---
> > > > > > >   fs/ceph/xattr.c | 13 +++++++------
> > > > > > >   1 file changed, 7 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> > > > > > > index fcf7dfdecf96..a61e1adb49a9 100644
> > > > > > > --- a/fs/ceph/xattr.c
> > > > > > > +++ b/fs/ceph/xattr.c
> > > > > > > @@ -1306,7 +1306,7 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > > > > >   			   struct ceph_acl_sec_ctx *as_ctx)
> > > > > > >   {
> > > > > > >   	struct ceph_pagelist *pagelist = as_ctx->pagelist;
> > > > > > > -	const char *name;
> > > > > > > +	const char *name = NULL;
> > > > > > >   	size_t name_len;
> > > > > > >   	int err;
> > > > > > > @@ -1319,6 +1319,12 @@ int ceph_security_init_secctx(struct dentry *dentry, umode_t mode,
> > > > > > >   		goto out;
> > > > > > >   	}
> > > > > > > +	/* No LSM configured? Do nothing. */
> > > > > > > +	if (!name) {
> > > > > > Excuse my french but that reads very riské. Can we somehow make
> > > > > > security_dentry_init_security() return an error number that caller's
> > > > > > like ceph can use to skip?
> > > > > > I think that returning 0 from security_dentry_init_security() and having
> > > > > > *name be returned as NULL is very fragile.
> > > > > Good point. In fact, I'm not sure why he didn't get an -EOPNOTSUPP
> > > > > return here:
> > > > > 
> > > > >          return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> > > > >                                  name, xattr_name, ctx, ctxlen);
> > > > > 
> > > > > It looks like the second argument is what should end up being returned
> > > > > when there are no entries in the security_hook_list. So it seems like
> > > > > maybe Stephen's security_hook_list was not empty, but didn't have a
> > > > > vector for dentry_init_security and returned the default 0 there?
> > > > Yeah, I came to the exact same analysis and subsequent question in:
> > > > https://lore.kernel.org/ceph-devel/20220125111111.irvr4dg3i56crqrs@wittgenstein
> > > > 
> > > > It seems to me that selinux support is compiled in in archlinux but no
> > > > policy loaded/enabled. I don't know if that means that the hooks is
> > > > called and by default returns 0 or something. I'm rather confused as the
> > > > call_int_hook() implies -EOPNOTSUPP is returned but looking at
> > > > dentry_init_security as defined in LSM_HOOKS(int, 0,
> > > > dentry_init_security, [...]) in include/linux/lsm_hook_defs.h implies
> > > > that the default return value is 0.
> > > Yeah, the problem is the call_int_hook macro, I think, which wants to
> > > walk through all of the LSMs in the stack and call this for all of
> > > them.  What happens though if you have 2 entries with valid
> > > dentry_init_security hooks in them, and they both try to set the xattr
> > > name?
> > > 
> > > This can only ever return a single xattr_name, so maybe the right thing
> > > to do is to make the default for the LSM_HOOK be -EOPNOTSUPP and add a
> > > macro that does call_int_hook, but returns after the first supported
> > > hook call? Something like this maybe? (untested):
> > [ cc Casey Schaufler <casey@schaufler-ca.com> ]
> 
> Joining the conversation late. Wish someone had brought me
> in sooner.
> 
> > Actually this patch might work. IIUC, so is the problem happening due
> > to BPF LSM (CONFIG_BPF_LSM). I see following in bpf_lsm.c
> > 
> > /* For every LSM hook that allows attachment of BPF programs, declare a nop
> >   * function where a BPF program can be attached.
> >   */
> > #define LSM_HOOK(RET, DEFAULT, NAME, ...)       \
> > noinline RET bpf_lsm_##NAME(__VA_ARGS__)        \
> > {                                               \
> >          return DEFAULT;                         \
> > }
> > 
> > So if CONFIG_BPF_LSM=y, then it will automatically register a hook
> > for dentry_init_security() and return DEFAULT. I suspect, that what
> > is happening here and that's how we get return code of 0 and not
> > -EOPNOTSUPP.
> > 
> > So LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, ....) should fix
> > that.
> > 
> > But you are right that now if both SELINUX and BPF_LSM are enabled
> > and in that case we will call both the hooks and call_int_hook()
> > will not work anymore if BPF LSM hook is called first. It will
> > return -EOPNOTSUPP without ever calling into SELinux lsm. So we
> > need the extra helper which continues to call into next LSM if
> > previous LSMs returned -EOPNOTSUPP.
> 
> This is a case where the LSM hook code needs to eschew call_int_hook()
> and open code the required logic. Look at security_inode_getsecurity().
> Please resist the temptation to add more macros.
> 
> > 
> > This will work as long as BPF LSM always returns -EOPNOTSUPP. I don't
> > know enough about BPF. But can one write a BPF program and return
> > something other than -EOPNOTSUPP. In that case it will break.
> > 
> > May be not. I guess if there are multiple LSMs supporting
> > dentry_init_security() hooks, then intent is to call into first
> > LSM which does something and initializes security context. If BPF
> > somehow manages to do that, so it gets that opportunity and SELinux
> > will not be called.
> > 
> > Having said that, SELinux might not like the idea.
> 
> BPF provides all LSM hooks, all the time. So long as BPF
> is stacked last you should be OK. The BPF developers claim
> that it has to be this way.

If this is true that BPF LSM is stacked last, then there should not
be any need to eschew call_int_hook(). Just use.

LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, ....)

and that should fix the issue, atleast with current code. As only two
LSMs (SELinux and BPF) seem to register handler for this.

Having said that, open coding like security_inode_getsecurity() does
not rely on this assumption that BPF LSM is always stacked last.

I will try to come up with a patch.

Thanks
Vivek

> 
> > 
> > And if SELinux is called first, then BPF hook will never be called
> > and any bpf program waiting to be called will be missed. I guess
> > this is lesser of a problem. First we need to fix ceph crash and
> > also make sure SELinux context is initialized (even if BPF LSM is
> > enabled).
> > 
> > Looks like dentry_init_security() can't handle multiple LSMs. We probably
> > should disallow all other LSMs to register a hook for this and only
> > allow SELinux to register a hook.
> 
> Not acceptable. The fix to dentry_init_security() is easy.
> 
> > 
> > Thanks
> > Vivek
> > 
> > 
> > 
> > > -----------8<----------------
> > > 
> > > [RFC PATCH] security: dentry_init_security hook should return on first success
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >   include/linux/lsm_hook_defs.h |  2 +-
> > >   security/security.c           | 18 ++++++++++++++++--
> > >   2 files changed, 17 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > > index a5a724c308d8..819ec92dc2a8 100644
> > > --- a/include/linux/lsm_hook_defs.h
> > > +++ b/include/linux/lsm_hook_defs.h
> > > @@ -80,7 +80,7 @@ LSM_HOOK(int, 0, sb_clone_mnt_opts, const struct super_block *oldsb,
> > >   	 unsigned long *set_kern_flags)
> > >   LSM_HOOK(int, 0, move_mount, const struct path *from_path,
> > >   	 const struct path *to_path)
> > > -LSM_HOOK(int, 0, dentry_init_security, struct dentry *dentry,
> > > +LSM_HOOK(int, -EOPNOTSUPP, dentry_init_security, struct dentry *dentry,
> > >   	 int mode, const struct qstr *name, const char **xattr_name,
> > >   	 void **ctx, u32 *ctxlen)
> > >   LSM_HOOK(int, 0, dentry_create_files_as, struct dentry *dentry, int mode,
> > > diff --git a/security/security.c b/security/security.c
> > > index 3d4eb474f35b..0133e60f4817 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -745,6 +745,19 @@ static int lsm_superblock_alloc(struct super_block *sb)
> > >   	RC;							\
> > >   })
> > > +#define call_first_supported_int_hook(FUNC, IRC, ...) ({	\
> > > +	int RC = IRC;						\
> > > +	do {							\
> > > +		struct security_hook_list *P;			\
> > > +								\
> > > +		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > > +			RC = P->hook.FUNC(__VA_ARGS__);		\
> > > +			if (RC != -EOPNOTSUPP)			\
> > > +				break;				\
> > > +		}						\
> > > +	} while (0);						\
> > > +	RC;							\
> > > +})
> > >   /* Security operations */
> > >   int security_binder_set_context_mgr(const struct cred *mgr)
> > > @@ -1048,8 +1061,9 @@ int security_dentry_init_security(struct dentry *dentry, int mode,
> > >   				  const char **xattr_name, void **ctx,
> > >   				  u32 *ctxlen)
> > >   {
> > > -	return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
> > > -				name, xattr_name, ctx, ctxlen);
> > > +	return call_first_supported_int_hook(dentry_init_security,
> > > +					     -EOPNOTSUPP, dentry, mode,
> > > +					     name, xattr_name, ctx, ctxlen);
> > >   }
> > >   EXPORT_SYMBOL(security_dentry_init_security);
> > > -- 
> > > 2.34.1
> > > 
> > > 
> > > 
> 

Re: "kernel NULL pointer dereference" crash when attempting a write

[Vivek Goyal]

On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:

[..]
> -	/*
> -	 * FIXME: Make security_dentry_init_security() generic. Currently
> -	 * It only supports single security module and only selinux has
> -	 * dentry_init_security hook.
> -	 */

I think this comment is still very valid. security_dentry_init_security()
still supports only single security module. It does not have the
capability to deal with two modules trying to return security context.

All my patch did was that instead of SELinux, now another LSM could
return security context and also return xattr name for the security
context. But it did nothing to allow multiple LSMs to return their
own security contexts.

IMHO, this FIXME should still be there and not removed. 

Thanks
Vivek

Re: "kernel NULL pointer dereference" crash when attempting a write

[Christian Brauner]

On Tue, Jan 25, 2022 at 08:54:24AM -0500, Vivek Goyal wrote:
> On Tue, Jan 25, 2022 at 05:54:57AM -0500, Jeff Layton wrote:
> 
> [..]
> > -	/*
> > -	 * FIXME: Make security_dentry_init_security() generic. Currently
> > -	 * It only supports single security module and only selinux has
> > -	 * dentry_init_security hook.
> > -	 */
> 
> I think this comment is still very valid. security_dentry_init_security()
> still supports only single security module. It does not have the
> capability to deal with two modules trying to return security context.
> 
> All my patch did was that instead of SELinux, now another LSM could
> return security context and also return xattr name for the security
> context. But it did nothing to allow multiple LSMs to return their
> own security contexts.

Yeah, that still doesn't work.
Your patch did however help uncover what is a but in the current
security_dentry_init_security() implementation afaict. Ceph was on the
safe side so far because it always initialized name unconditionally
because it knew that only selinux was relevant for this.

Re: "kernel NULL pointer dereference" crash when attempting a write

[Christian Brauner]

On Mon, Jan 24, 2022 at 09:45:10PM -0500, Paul Moore wrote:
> On Mon, Jan 24, 2022 at 8:51 PM Stephen Muth <smuth4@gmail.com> wrote:
> >
> > Hello,
> >
> > I'm seeing an Oops in the kernel whenever I try to make changes (e.g. `touch`ing a file) in a cephfs mount on Arch running kernel 5.16.2. Reading works fine up until a modification is attempted, and then all IO will start hanging.
> >
> > This appears to be directly related to "security: Return xattr name from security_dentry_init_security()" (commit 15bf323), as recompiling with just that change reverted makes the issue go away. The same cluster is actively receiving writes from older kernels (5.13), and it was initially reported at https://bugs.archlinux.org/task/73408, where a user reports that 5.15 kernels also work.
> >
> > /proc/version: Linux version 5.16.2-arch1-1 (linux@archlinux) (gcc (GCC) 11.1.0, GNU ld (GNU Binutils) 2.36.1) #1 SMP PREEMPT Thu, 20 Jan 2022 16:18:29 +0000
> > distro / arch: Arch Linux / x86_64
> > SELinux is not enabled
> > ceph cluster version: 16.2.7 (dd0603118f56ab514f133c8d2e3adfc983942503)
> >
> > relevant dmesg output:
> > [   30.947129] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [   30.947206] #PF: supervisor read access in kernel mode
> > [   30.947258] #PF: error_code(0x0000) - not-present page
> > [   30.947310] PGD 0 P4D 0
> > [   30.947342] Oops: 0000 [#1] PREEMPT SMP PTI
> > [   30.947388] CPU: 5 PID: 778 Comm: touch Not tainted 5.16.2-arch1-1 #1 86fbf2c313cc37a553d65deb81d98e9dcc2a3659
> > [   30.947486] Hardware name: Gigabyte Technology Co., Ltd. B365M DS3H/B365M DS3H, BIOS F5 08/13/2019
> > [   30.947569] RIP: 0010:strlen+0x0/0x20
> > [   30.947616] Code: b6 07 38 d0 74 16 48 83 c7 01 84 c0 74 05 48 39 f7 75 ec 31 c0 31 d2 89 d6 89 d7 c3 48 89 f8 31 d2 89 d6 89 d7 c3 0
> > f 1f 40 00 <80> 3f 00 74 12 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 31 ff
> > [   30.947782] RSP: 0018:ffffa4ed80ffbbb8 EFLAGS: 00010246
> > [   30.947836] RAX: 0000000000000000 RBX: ffffa4ed80ffbc60 RCX: 0000000000000000
> > [   30.947904] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> > [   30.947971] RBP: ffff94b0d15c0ae0 R08: 0000000000000000 R09: 0000000000000000
> > [   30.948040] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> > [   30.948106] R13: 0000000000000001 R14: ffffa4ed80ffbc60 R15: 0000000000000000
> > [   30.948174] FS:  00007fc7520f0740(0000) GS:ffff94b7ced40000(0000) knlGS:0000000000000000
> > [   30.948252] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   30.948308] CR2: 0000000000000000 CR3: 0000000104a40001 CR4: 00000000003706e0
> > [   30.948376] Call Trace:
> > [   30.948404]  <TASK>
> > [   30.948431]  ceph_security_init_secctx+0x7b/0x240 [ceph 49f9c4b9bf5be8760f19f1747e26da33920bce4b]
> 
> My guess is that the "name_len = strlen(name);" line in
> ceph_security_init_secctx() is causing the problem as "name" is likely
> NULL/garbage without any LSMs configured for that hook.

Afaict, the following things need to be true simultaneously:

1. kernel compiled with selinux support
2. not selinux policy loaded
3. security_dentry_init_security() returns 0
4. security_dentry_init_security did not fill in *name

Ideally security_dentry_init_security() would return an err value of
some format indicating that nothing happened instead of relying on
callers checking for NULL. I somehow thought it would return
-EOPNOTSUPP.

> 
> [SIDE NOTE: we should remove the comment block directly above that
> line as it no longer applies, arguably commit 15bf32398ad4 should have
> removed it.]
> 
> I'm open to suggestions on the best approach, but my gut feeling is
> that the fix is to have the security_dentry_init_security() hook set
> the xattr_name parameter to NULL before calling into the LSMs and then
> the callers should check for "name == NULL" on return.  In the case of
> ceph that would probably be something like this:
> 
>   name_len = (name ? strlen(name) : 0);
> 
> ... with fuse looking very similar, although fuse appears to need a
> "strlen(name) + 1".  We don't have to worry about this for NFSv4 as it
> doesn't use the xattr name.
> 
> Thoughts?