ocfs2: check if cluster name exists before deref

PATCH New Sasha Levin 3 participants 7 messages Mar 25, 2014 – Mar 26, 2014 OCFS2

Share & tools

Sasha Levin

author

2014-03-25 17:21 UTC #0 lore raw

PATCH 1/1 ocfs2: check if cluster name exists before deref

Commit c74a3bdd9b "ocfs2: add clustername to cluster connection" is trying to strlcpy a string which was explicitly passed as NULL in the very same patch, triggering a NULL ptr deref.

[  640.225193] BUG: unable to handle kernel NULL pointer dereference at           (null)
[  640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)
[  640.230224] PGD 82a93a067 PUD 82a93b067 PMD 0 
[  640.230224] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  640.230224] Dumping ftrace buffer:
[  640.230224]    (ftrace buffer empty)
[  640.230224] Modules linked in:
[  640.230224] CPU: 19 PID: 19426 Comm: trinity-c19 Tainted: G        W     3.14.0-rc7-next-20140325-sasha-00014-g9476368-dirty #274
[  640.230224] task: ffff88082bc53000 ti: ffff88082b674000 task.ti: ffff88082b674000
[  640.230224] RIP:  strlcpy (lib/string.c:388 lib/string.c:151)
[  640.230224] RSP: 0018:ffff88082b675d88  EFLAGS: 00010296
[  640.230224] RAX: 0000000000000007 RBX: ffffffff8853b260 RCX: 000000006f6d7366
[  640.230224] RDX: 0000000000000011 RSI: 0000000000000000 RDI: ffff88052bcd3518  
[  640.230224] RBP: ffff88082b675da8 R08: 00000000746e756f R09: 0000000000000000  
[  640.230224] R10: ffff88052bcd34d0 R11: 0000000000000000 R12: ffff88052bcd3518  
[  640.230224] R13: ffff88052c003fb8 R14: ffff88052bcd34d0 R15: 00000000ffffffea
[  640.230224] FS:  00007f04ae7a6700(0000) GS:ffff88052cc00000(0000) knlGS:0000000000000000
[  640.230224] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  640.230224] CR2: 0000000000000000 CR3: 000000082115b000 CR4: 00000000000006a0
[  640.230224] DR0: 0000000000698000 DR1: 0000000000698000 DR2: 0000000000000000
[  640.230224] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000602
[  640.230224] Stack:
[  640.230224]  ffffffff86b3c260 ffffffff8853b260 ffffffff86b3c260 ffff88052c003fb8
[  640.230224]  ffff88082b675df8 ffffffff818a3a5d 0000000000000000 0000000700000000
[  640.230224]  0000000000000282 ffff88052c003f48 ffff88003e6b01a0 ffff88052c0f81a0
[  640.230224] Call Trace:
[  640.230224]  ocfs2_cluster_connect (fs/ocfs2/stackglue.c:350)
[  640.230224]  ocfs2_cluster_connect_agnostic (fs/ocfs2/stackglue.c:396)
[  640.230224]  ? ocfs2_control_open (fs/ocfs2/dlmfs/userdlm.c:660)
[  640.230224]  user_dlm_register (fs/ocfs2/dlmfs/userdlm.c:679)
[  640.230224]  ? dlmfs_get_inode (fs/ocfs2/dlmfs/dlmfs.c:468)
[  640.230224]  dlmfs_mkdir (fs/ocfs2/dlmfs/dlmfs.c:503)
[  640.230224]  ? security_inode_permission (security/security.c:555)
[  640.230224]  ? __inode_permission (fs/namei.c:414)
[  640.230224]  vfs_mkdir (fs/namei.c:3467)
[  640.230224]  SyS_mkdirat (fs/namei.c:3488 fs/namei.c:3472)
[  640.230224]  tracesys (arch/x86/kernel/entry_64.S:749)  
[  640.230224] Code: 41 c6 44 1d 00 00 48 83 c4 08 5b 4c 89 e0 41 5c 41 5d 5d c3 0f 1f 80 00 00 00 00 55 48 89 e5 41 55 41 54 49 89 fc 53 48 83 ec 08 <80> 3e 00 74 1c 48 89 f0 0f 1f 84 00 00 00 00 00 48 83 c0 01 80 
[  640.230224] RIP  strlcpy (lib/string.c:388 lib/string.c:151)
[  640.230224]  RSP <ffff88082b675d88>
[  640.230224] CR2: 0000000000000000

Signed-off-by Sasha Levin <[email protected]>

---

As a side note, how the hell was this new code path tested? It's obviously broken and there's no way it even passes a very basic test.

 fs/ocfs2/stackglue.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

fs/ocfs2/stackglue.c +3 -1

index 5e4d314..83f1a66 100644
@@ -346,7 +346,9 @@ int ocfs2_cluster_connect(const char *stack_name,
 
 	strlcpy(new_conn->cc_name, group, GROUP_NAME_MAX + 1);
 	new_conn->cc_namelen = grouplen;
-	strlcpy(new_conn->cc_cluster_name, cluster_name, CLUSTER_NAME_MAX + 1);
+	if (cluster_name_len)
+		strlcpy(new_conn->cc_cluster_name, cluster_name,
+			CLUSTER_NAME_MAX + 1);
 	new_conn->cc_cluster_name_len = cluster_name_len;
 	new_conn->cc_recovery_handler = recovery_handler;
 	new_conn->cc_recovery_data = recovery_data;

Signature · 1 line

1.7.10.4

Andrew Morton

2014-03-26 21:33 UTC #1 lore raw

PATCH 1/1 ocfs2: check if cluster name exists before deref

Quoted 6 lines · On Tue, 25 Mar 2014 13:21:58 -0400 Sasha Levin <[email protected]> wrote:

Commit c74a3bdd9b "ocfs2: add clustername to cluster connection" is trying to strlcpy a string which was explicitly passed as NULL in the very same patch, triggering a NULL ptr deref.
[  640.225193] BUG: unable to handle kernel NULL pointer dereference at           (null)
[  640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)

Well that was a bit of a screwup.

Quoted 3 lines

As a side note, how the hell was this new code path tested? It's obviously broken and there's no way it even passes a very basic test.

I was wondering that.

Quoted 5 lines

fs/ocfs2/stackglue.c +0 -0
index 5e4d314..83f1a66 100644

fs/ocfs2/stackglue.c int ocfs2_cluster_connect(const char *stack_name, · line 354

Quoted 10 lines

fs/ocfs2/stackglue.c +3 -1

@@ -346,7 +346,9 @@ int ocfs2_cluster_connect(const char *stack_name,
 
 	strlcpy(new_conn->cc_name, group, GROUP_NAME_MAX + 1);
 	new_conn->cc_namelen = grouplen;
-	strlcpy(new_conn->cc_cluster_name, cluster_name, CLUSTER_NAME_MAX + 1);
+	if (cluster_name_len)
+		strlcpy(new_conn->cc_cluster_name, cluster_name,
+			CLUSTER_NAME_MAX + 1);
 	new_conn->cc_cluster_name_len = cluster_name_len;
 	new_conn->cc_recovery_handler = recovery_handler;
 	new_conn->cc_recovery_data = recovery_data;

So we end up with the null string for the cluster name.

I suppose we can merge this short-term to avoid the oops, but surely this isn't what's supposed to happen.

Goldwyn, this needs urgent attention please.

Mark Fasheh

2014-03-26 21:43 UTC #2 lore raw

PATCH 1/1 ocfs2: check if cluster name exists before deref

CCing Goldywn so he actually gets this e-mail :)

Quoted 38 lines · Andrew Morton

Quoted 6 lines · On Tue, 25 Mar 2014 13:21:58 -0400 Sasha Levin <[email protected]> wrote:
Commit c74a3bdd9b "ocfs2: add clustername to cluster connection" is trying to strlcpy a string which was explicitly passed as NULL in the very same patch, triggering a NULL ptr deref.
[  640.225193] BUG: unable to handle kernel NULL pointer dereference at           (null)
[  640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)
Well that was a bit of a screwup.
Quoted 3 lines
As a side note, how the hell was this new code path tested? It's obviously broken and there's no way it even passes a very basic test.
I was wondering that.
Quoted 5 lines
fs/ocfs2/stackglue.c +0 -0
index 5e4d314..83f1a66 100644
fs/ocfs2/stackglue.c int ocfs2_cluster_connect(const char *stack_name, · line 354
Quoted 10 lines
fs/ocfs2/stackglue.c +3 -1
@@ -346,7 +346,9 @@ int ocfs2_cluster_connect(const char *stack_name,
 
 	strlcpy(new_conn->cc_name, group, GROUP_NAME_MAX + 1);
 	new_conn->cc_namelen = grouplen;
-	strlcpy(new_conn->cc_cluster_name, cluster_name, CLUSTER_NAME_MAX + 1);
+	if (cluster_name_len)
+		strlcpy(new_conn->cc_cluster_name, cluster_name,
+			CLUSTER_NAME_MAX + 1);
 	new_conn->cc_cluster_name_len = cluster_name_len;
 	new_conn->cc_recovery_handler = recovery_handler;
 	new_conn->cc_recovery_data = recovery_data;
So we end up with the null string for the cluster name.
I suppose we can merge this short-term to avoid the oops, but surely this isn't what's supposed to happen.

Quoted 6 lines

Goldwyn, this needs urgent attention please.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

--
Mark Fasheh

Mark Fasheh

2014-03-26 22:00 UTC #3 lore raw

PATCH 1/1 ocfs2: check if cluster name exists before deref

In reply to Sasha Levin

Quoted 46 lines · Sasha Levin

Commit c74a3bdd9b "ocfs2: add clustername to cluster connection" is trying to strlcpy a string which was explicitly passed as NULL in the very same patch, triggering a NULL ptr deref.

[  640.225193] BUG: unable to handle kernel NULL pointer dereference at           (null)
[  640.230224] IP: strlcpy (lib/string.c:388 lib/string.c:151)
[  640.230224] PGD 82a93a067 PUD 82a93b067 PMD 0 
[  640.230224] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  640.230224] Dumping ftrace buffer:
[  640.230224]    (ftrace buffer empty)
[  640.230224] Modules linked in:
[  640.230224] CPU: 19 PID: 19426 Comm: trinity-c19 Tainted: G        W     3.14.0-rc7-next-20140325-sasha-00014-g9476368-dirty #274
[  640.230224] task: ffff88082bc53000 ti: ffff88082b674000 task.ti: ffff88082b674000
[  640.230224] RIP:  strlcpy (lib/string.c:388 lib/string.c:151)
[  640.230224] RSP: 0018:ffff88082b675d88  EFLAGS: 00010296
[  640.230224] RAX: 0000000000000007 RBX: ffffffff8853b260 RCX: 000000006f6d7366
[  640.230224] RDX: 0000000000000011 RSI: 0000000000000000 RDI: ffff88052bcd3518  
[  640.230224] RBP: ffff88082b675da8 R08: 00000000746e756f R09: 0000000000000000  
[  640.230224] R10: ffff88052bcd34d0 R11: 0000000000000000 R12: ffff88052bcd3518  
[  640.230224] R13: ffff88052c003fb8 R14: ffff88052bcd34d0 R15: 00000000ffffffea
[  640.230224] FS:  00007f04ae7a6700(0000) GS:ffff88052cc00000(0000) knlGS:0000000000000000
[  640.230224] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  640.230224] CR2: 0000000000000000 CR3: 000000082115b000 CR4: 00000000000006a0
[  640.230224] DR0: 0000000000698000 DR1: 0000000000698000 DR2: 0000000000000000
[  640.230224] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000602
[  640.230224] Stack:
[  640.230224]  ffffffff86b3c260 ffffffff8853b260 ffffffff86b3c260 ffff88052c003fb8
[  640.230224]  ffff88082b675df8 ffffffff818a3a5d 0000000000000000 0000000700000000
[  640.230224]  0000000000000282 ffff88052c003f48 ffff88003e6b01a0 ffff88052c0f81a0
[  640.230224] Call Trace:
[  640.230224]  ocfs2_cluster_connect (fs/ocfs2/stackglue.c:350)
[  640.230224]  ocfs2_cluster_connect_agnostic (fs/ocfs2/stackglue.c:396)
[  640.230224]  ? ocfs2_control_open (fs/ocfs2/dlmfs/userdlm.c:660)
[  640.230224]  user_dlm_register (fs/ocfs2/dlmfs/userdlm.c:679)
[  640.230224]  ? dlmfs_get_inode (fs/ocfs2/dlmfs/dlmfs.c:468)
[  640.230224]  dlmfs_mkdir (fs/ocfs2/dlmfs/dlmfs.c:503)
[  640.230224]  ? security_inode_permission (security/security.c:555)
[  640.230224]  ? __inode_permission (fs/namei.c:414)
[  640.230224]  vfs_mkdir (fs/namei.c:3467)
[  640.230224]  SyS_mkdirat (fs/namei.c:3488 fs/namei.c:3472)
[  640.230224]  tracesys (arch/x86/kernel/entry_64.S:749)  
[  640.230224] Code: 41 c6 44 1d 00 00 48 83 c4 08 5b 4c 89 e0 41 5c 41 5d 5d c3 0f 1f 80 00 00 00 00 55 48 89 e5 41 55 41 54 49 89 fc 53 48 83 ec 08 <80> 3e 00 74 1c 48 89 f0 0f 1f 84 00 00 00 00 00 48 83 c0 01 80 
[  640.230224] RIP  strlcpy (lib/string.c:388 lib/string.c:151)
[  640.230224]  RSP <ffff88082b675d88>
[  640.230224] CR2: 0000000000000000

Quoted 6 lines

Signed-off-by Sasha Levin <[email protected]>
---
As a side note, how the hell was this new code path tested? It's obviously broken and there's no way it even passes a very basic test.

I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made - it's a fact of life. We have a review process to hopefully catch these sorts of things, you are welcome to take part in it. In fact, if you were willing to test and report these sorts of things to the Ocfs2-devel list before they go upstream that would probably help prevent this sort of thing from happening in the future.

Thanks,

	--Mark

--
Mark Fasheh

Andrew Morton

2014-03-26 22:08 UTC #4 lore raw

PATCH 1/1 ocfs2: check if cluster name exists before deref

Quoted 10 lines · On Wed, 26 Mar 2014 15:00:18 -0700 Mark Fasheh <[email protected]> wrote:

Quoted 3 lines
As a side note, how the hell was this new code path tested? It's obviously broken and there's no way it even passes a very basic test.
I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made - it's a fact of life. We have a review process to hopefully catch these sorts of things, you are welcome to take part in it. In fact, if you were willing to test and report these sorts of things to the Ocfs2-devel list before they go upstream that would probably help prevent this sort of thing from happening in the future.

Well. Sasha is the kernel's reigning testing-and-reporting champion. On linux-next, no less.

What concerns me is that the wrong patch version was merged, or it got mangled in flight somehow.

Mark Fasheh

2014-03-26 22:23 UTC #5 lore raw

PATCH 1/1 ocfs2: check if cluster name exists before deref

Quoted 13 lines · Andrew Morton

Quoted 10 lines · On Wed, 26 Mar 2014 15:00:18 -0700 Mark Fasheh <[email protected]> wrote:
Quoted 3 lines
As a side note, how the hell was this new code path tested? It's obviously broken and there's no way it even passes a very basic test.
I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made - it's a fact of life. We have a review process to hopefully catch these sorts of things, you are welcome to take part in it. In fact, if you were willing to test and report these sorts of things to the Ocfs2-devel list before they go upstream that would probably help prevent this sort of thing from happening in the future.

Quoted 2 lines

Well. Sasha is the kernel's reigning testing-and-reporting champion. On linux-next, no less.

Huh, Sasha, are you testing Ocfs2 from linux-next? If so some details of how / what you test would be appreciated. Otherwise, perhaps we could talk about some basic Ocfs2 tests then?

Quoted 2 lines

What concerns me is that the wrong patch version was merged, or it got mangled in flight somehow.

I kind of assumed that must've been what happened, but probably on Goldwyn's end - that he sent the wrong version of the patch or something. It's not like that hasn't happened to me before, etc. In flight would be weird for me to hear though, I've never experienced that happening.

	--Mark

--
Mark Fasheh

Sasha Levin

author

2014-03-26 22:23 UTC #6 lore raw

PATCH 1/1 ocfs2: check if cluster name exists before deref

In reply to Mark Fasheh

Quoted 11 lines · Mark Fasheh

Quoted 3 lines · Sasha Levin
As a side note, how the hell was this new code path tested? It's obviously broken and there's no way it even passes a very basic test.
I'm not trying to 'defend' Goldwyn, or anything, but mistakes get made - it's a fact of life. We have a review process to hopefully catch these sorts of things, you are welcome to take part in it. In fact, if you were willing to test and report these sorts of things to the Ocfs2-devel list before they go upstream that would probably help prevent this sort of thing from happening in the future.

Hi Mark,

I'm fuzz testing the kernel and not really testing or reviewing any subsystem in particular.

My frustration about not testing it was because it's so simple to reproduce this issue:

  # mount -t ocfs2_dlmfs ocfs2_dlmfs dlm
  # mkdir dlm/anything
  [ boom ]

This is a very basic test that should have been run more than once before this patch made it into Linus's tree. Whether as part of some ocfs2 test suite or maybe xfstest.

My beef is not with Goldwyn not testing that path, it's more with whatever process (or lack thereof) ocfs2 has to test and verify patches before they make it upstream. Even if Goldwyn missed it, it should have been caught at a different place on it's way.

Thanks,
Sasha