nfsd: Fix oops when parsing a 0 length export

PATCH In review Sasha Levin 3 participants 11 messages Nov 18, 2011 – Jan 20, 2012

Subsystems (3)

Networking NFS client NFS server

lore

Share & tools

lore

Sasha Levin

author

2011-11-18 10:15 UTC #0 lore raw

PATCH 1/1 nfsd: Fix oops when parsing a 0 length export

expkey_parse() oopses when handling a 0 length export. This is easily triggerable from usermode by writing 0 bytes into '/proc/[proc id]/net/rpc/nfsd.fh/channel'.

Below is the log:

[ 1402.286893] BUG: unable to handle kernel paging request at ffff880077c49fff
[ 1402.287632] IP: [<ffffffff812b4b99>] expkey_parse+0x28/0x2e1
[ 1402.287632] PGD 2206063 PUD 1fdfd067 PMD 1ffbc067 PTE 8000000077c49160
[ 1402.287632] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 1402.287632] CPU 1
[ 1402.287632] Pid: 20198, comm: trinity Not tainted 3.2.0-rc2-sasha-00058-gc65cd37 #6
[ 1402.287632] RIP: 0010:[<ffffffff812b4b99>]  [<ffffffff812b4b99>] expkey_parse+0x28/0x2e1
[ 1402.287632] RSP: 0018:ffff880077f0fd68  EFLAGS: 00010292
[ 1402.287632] RAX: ffff880077c49fff RBX: 00000000ffffffea RCX: 0000000001043400
[ 1402.287632] RDX: 0000000000000000 RSI: ffff880077c4a000 RDI: ffffffff82283de0
[ 1402.287632] RBP: ffff880077f0fe18 R08: 0000000000000001 R09: ffff880000000000
[ 1402.287632] R10: 0000000000000000 R11: 0000000000000001 R12: ffff880077c4a000
[ 1402.287632] R13: ffffffff82283de0 R14: 0000000001043400 R15: ffffffff82283de0
[ 1402.287632] FS:  00007f25fec3f700(0000) GS:ffff88007d400000(0000) knlGS:0000000000000000
[ 1402.287632] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1402.287632] CR2: ffff880077c49fff CR3: 0000000077e1d000 CR4: 00000000000406e0
[ 1402.287632] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1402.287632] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1402.287632] Process trinity (pid: 20198, threadinfo ffff880077f0e000, task ffff880077db17b0)
[ 1402.287632] Stack:
[ 1402.287632]  ffff880077db17b0 ffff880077c4a000 ffff880077f0fdb8 ffffffff810b411e
[ 1402.287632]  ffff880000000000 ffff880077db17b0 ffff880077c4a000 ffffffff82283de0
[ 1402.287632]  0000000001043400 ffffffff82283de0 ffff880077f0fde8 ffffffff81111f63
[ 1402.287632] Call Trace:
[ 1402.287632]  [<ffffffff810b411e>] ? lock_release+0x1af/0x1bc
[ 1402.287632]  [<ffffffff81111f63>] ? might_fault+0x97/0x9e
[ 1402.287632]  [<ffffffff81111f1a>] ? might_fault+0x4e/0x9e
[ 1402.287632]  [<ffffffff81a8bcf2>] cache_do_downcall+0x3e/0x4f
[ 1402.287632]  [<ffffffff81a8c950>] cache_write.clone.16+0xbb/0x130
[ 1402.287632]  [<ffffffff81a8c9df>] ? cache_write_pipefs+0x1a/0x1a
[ 1402.287632]  [<ffffffff81a8c9f8>] cache_write_procfs+0x19/0x1b
[ 1402.287632]  [<ffffffff8118dc54>] proc_reg_write+0x8e/0xad
[ 1402.287632]  [<ffffffff8113fe81>] vfs_write+0xaa/0xfd
[ 1402.287632]  [<ffffffff8114142d>] ? fget_light+0x35/0x9e
[ 1402.287632]  [<ffffffff8113ff8b>] sys_write+0x48/0x6f
[ 1402.287632]  [<ffffffff81bbdb92>] system_call_fastpath+0x16/0x1b
[ 1402.287632] Code: c0 c9 c3 55 48 63 d2 48 89 e5 48 8d 44 32 ff 41 57 41 56 41 55 41 54 53 bb ea ff ff ff 48 81 ec 88 00 00 00 48 89 b5 58 ff ff ff
[ 1402.287632]  38 0a 0f 85 89 02 00 00 c6 00 00 48 8b 3d 44 4a e5 01 48 85
[ 1402.287632] RIP  [<ffffffff812b4b99>] expkey_parse+0x28/0x2e1
[ 1402.287632]  RSP <ffff880077f0fd68>
[ 1402.287632] CR2: ffff880077c49fff
[ 1402.287632] ---[ end trace 368ef53ff773a5e3 ]---

Cc "J. Bruce Fields" <[email protected]>

Cc Neil Brown <[email protected]>

Cc [email protected]

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

---
 fs/nfsd/export.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/nfsd/export.c +1 -1

index 62f3b90..5f312ab 100644
@@ -87,7 +87,7 @@ static int expkey_parse(struct cache_detail *cd, char *mesg, int mlen)
 	struct svc_expkey key;
 	struct svc_expkey *ek = NULL;
 
-	if (mesg[mlen-1] != '\n')
+	if (mlen < 1 || mesg[mlen-1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;

Signature · 1 line

1.7.8.rc1

J. Bruce Fields

2011-11-22 20:28 UTC #1 lore raw

Quoted 3 lines · Sasha Levin

expkey_parse() oopses when handling a 0 length export. This is easily triggerable from usermode by writing 0 bytes into '/proc/[proc id]/net/rpc/nfsd.fh/channel'.

Thanks--how did you run across this?

--b.

Quoted 74 lines

Below is the log:

[ 1402.286893] BUG: unable to handle kernel paging request at ffff880077c49fff
[ 1402.287632] IP: [<ffffffff812b4b99>] expkey_parse+0x28/0x2e1
[ 1402.287632] PGD 2206063 PUD 1fdfd067 PMD 1ffbc067 PTE 8000000077c49160
[ 1402.287632] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[ 1402.287632] CPU 1
[ 1402.287632] Pid: 20198, comm: trinity Not tainted 3.2.0-rc2-sasha-00058-gc65cd37 #6
[ 1402.287632] RIP: 0010:[<ffffffff812b4b99>]  [<ffffffff812b4b99>] expkey_parse+0x28/0x2e1
[ 1402.287632] RSP: 0018:ffff880077f0fd68  EFLAGS: 00010292
[ 1402.287632] RAX: ffff880077c49fff RBX: 00000000ffffffea RCX: 0000000001043400
[ 1402.287632] RDX: 0000000000000000 RSI: ffff880077c4a000 RDI: ffffffff82283de0
[ 1402.287632] RBP: ffff880077f0fe18 R08: 0000000000000001 R09: ffff880000000000
[ 1402.287632] R10: 0000000000000000 R11: 0000000000000001 R12: ffff880077c4a000
[ 1402.287632] R13: ffffffff82283de0 R14: 0000000001043400 R15: ffffffff82283de0
[ 1402.287632] FS:  00007f25fec3f700(0000) GS:ffff88007d400000(0000) knlGS:0000000000000000
[ 1402.287632] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 1402.287632] CR2: ffff880077c49fff CR3: 0000000077e1d000 CR4: 00000000000406e0
[ 1402.287632] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1402.287632] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1402.287632] Process trinity (pid: 20198, threadinfo ffff880077f0e000, task ffff880077db17b0)
[ 1402.287632] Stack:
[ 1402.287632]  ffff880077db17b0 ffff880077c4a000 ffff880077f0fdb8 ffffffff810b411e
[ 1402.287632]  ffff880000000000 ffff880077db17b0 ffff880077c4a000 ffffffff82283de0
[ 1402.287632]  0000000001043400 ffffffff82283de0 ffff880077f0fde8 ffffffff81111f63
[ 1402.287632] Call Trace:
[ 1402.287632]  [<ffffffff810b411e>] ? lock_release+0x1af/0x1bc
[ 1402.287632]  [<ffffffff81111f63>] ? might_fault+0x97/0x9e
[ 1402.287632]  [<ffffffff81111f1a>] ? might_fault+0x4e/0x9e
[ 1402.287632]  [<ffffffff81a8bcf2>] cache_do_downcall+0x3e/0x4f
[ 1402.287632]  [<ffffffff81a8c950>] cache_write.clone.16+0xbb/0x130
[ 1402.287632]  [<ffffffff81a8c9df>] ? cache_write_pipefs+0x1a/0x1a
[ 1402.287632]  [<ffffffff81a8c9f8>] cache_write_procfs+0x19/0x1b
[ 1402.287632]  [<ffffffff8118dc54>] proc_reg_write+0x8e/0xad
[ 1402.287632]  [<ffffffff8113fe81>] vfs_write+0xaa/0xfd
[ 1402.287632]  [<ffffffff8114142d>] ? fget_light+0x35/0x9e
[ 1402.287632]  [<ffffffff8113ff8b>] sys_write+0x48/0x6f
[ 1402.287632]  [<ffffffff81bbdb92>] system_call_fastpath+0x16/0x1b
[ 1402.287632] Code: c0 c9 c3 55 48 63 d2 48 89 e5 48 8d 44 32 ff 41 57 41 56 41 55 41 54 53 bb ea ff ff ff 48 81 ec 88 00 00 00 48 89 b5 58 ff ff ff
[ 1402.287632]  38 0a 0f 85 89 02 00 00 c6 00 00 48 8b 3d 44 4a e5 01 48 85
[ 1402.287632] RIP  [<ffffffff812b4b99>] expkey_parse+0x28/0x2e1
[ 1402.287632]  RSP <ffff880077f0fd68>
[ 1402.287632] CR2: ffff880077c49fff
[ 1402.287632] ---[ end trace 368ef53ff773a5e3 ]---

Cc "J. Bruce Fields" <[email protected]>

Cc Neil Brown <[email protected]>

Cc [email protected]

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

---
 fs/nfsd/export.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/nfsd/export.c +1 -1

index 62f3b90..5f312ab 100644
@@ -87,7 +87,7 @@ static int expkey_parse(struct cache_detail *cd, char *mesg, int mlen)
 	struct svc_expkey key;
 	struct svc_expkey *ek = NULL;
 
-	if (mesg[mlen-1] != '\n')
+	if (mlen < 1 || mesg[mlen-1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;

Signature · 6 lines

1.7.8.rc1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sasha Levin

author

2011-11-22 20:35 UTC #2 lore raw

Quoted 8 lines · J. Bruce Fields

Quoted 3 lines · Sasha Levin
expkey_parse() oopses when handling a 0 length export. This is easily triggerable from usermode by writing 0 bytes into '/proc/[proc id]/net/rpc/nfsd.fh/channel'.
Thanks--how did you run across this?
--b.

Tried running a fuzzer on the system.

Signature · 1 line

Sasha.

J. Bruce Fields

2011-11-22 20:59 UTC #3 lore raw

Quoted 11 lines · Sasha Levin

Quoted 8 lines · J. Bruce Fields
Quoted 3 lines · Sasha Levin
expkey_parse() oopses when handling a 0 length export. This is easily triggerable from usermode by writing 0 bytes into '/proc/[proc id]/net/rpc/nfsd.fh/channel'.
Thanks--how did you run across this?
--b.
Tried running a fuzzer on the system.

OK, thanks. As far as I can tell this could only be hit by a privileged user doing something stupid. And the bug has been there forever.

So I'm inclined just to queue this up for 3.3.

--b.

Dan Carpenter

2012-01-16 11:53 UTC #4 lore raw

PATCH 1/1 nfsd: oopses in cache_parse()

In reply to Sasha Levin

We fixed expkey_parse() in b2ea70afad "nfsd: Fix oops when parsing a 0 length export" but there are other cache_parse() implimentations which have the same issue.

Signed-off-by Dan Carpenter <[email protected]>

---
Since half of the implimentations get this wrong, maybe we should just check for this in cache_do_downcall(). Is there ever a valid reason to pass a zero length string to cache_parse()?

fs/nfsd/export.c +1 -1

index cf8a6bd..1d147a8 100644
@@ -496,7 +496,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 	struct svc_export exp = {}, *expp;
 	int an_int;
 
-	if (mesg[mlen-1] != '\n')
+	if (mlen < 1 || mesg[mlen - 1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;

fs/nfs/dns_resolve.c +1 -1

index a6e711a..d945d71 100644
@@ -217,7 +217,7 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
 	ssize_t len;
 	int ret = -EINVAL;
 
-	if (buf[buflen-1] != '\n')
+	if (buflen < 1 || buf[buflen - 1] != '\n')
 		goto out;
 	buf[buflen-1] = '\0';

fs/nfsd/nfs4idmap.c +2 -2

index 9409627..f8456a4 100644
@@ -202,7 +202,7 @@ idtoname_parse(struct cache_detail *cd, char *buf, int buflen)
 	int len;
 	int error = -EINVAL;
 
-	if (buf[buflen - 1] != '\n')
+	if (buflen < 1 || buf[buflen - 1] != '\n')
 		return (-EINVAL);
 	buf[buflen - 1]= '\0';

@@ -378,7 +378,7 @@ nametoid_parse(struct cache_detail *cd, char *buf, int buflen)
 	char *buf1;
 	int error = -EINVAL;
 
-	if (buf[buflen - 1] != '\n')
+	if (buflen < 1 || buf[buflen - 1] != '\n')
 		return (-EINVAL);
 	buf[buflen - 1]= '\0';

J. Bruce Fields

2012-01-16 15:50 UTC #5 lore raw

Re: [patch] nfsd: oopses in cache_parse()

Quoted 9 lines · Dan Carpenter

We fixed expkey_parse() in b2ea70afad "nfsd: Fix oops when parsing a 0 length export" but there are other cache_parse() implimentations which have the same issue.
Signed-off-by Dan Carpenter <[email protected]>
---
Since half of the implimentations get this wrong, maybe we should just check for this in cache_do_downcall(). Is there ever a valid reason to pass a zero length string to cache_parse()?

I don't think so, no. Checking in cache_do_downcall() sounds like a good idea.

--b.

Quoted 50 lines

fs/nfsd/export.c +1 -1

index cf8a6bd..1d147a8 100644
@@ -496,7 +496,7 @@ static int svc_export_parse(struct cache_detail *cd, char *mesg, int mlen)
 	struct svc_export exp = {}, *expp;
 	int an_int;
 
-	if (mesg[mlen-1] != '\n')
+	if (mlen < 1 || mesg[mlen - 1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;

fs/nfs/dns_resolve.c +1 -1

index a6e711a..d945d71 100644
@@ -217,7 +217,7 @@ static int nfs_dns_parse(struct cache_detail *cd, char *buf, int buflen)
 	ssize_t len;
 	int ret = -EINVAL;
 
-	if (buf[buflen-1] != '\n')
+	if (buflen < 1 || buf[buflen - 1] != '\n')
 		goto out;
 	buf[buflen-1] = '\0';

fs/nfsd/nfs4idmap.c +2 -2

index 9409627..f8456a4 100644
@@ -202,7 +202,7 @@ idtoname_parse(struct cache_detail *cd, char *buf, int buflen)
 	int len;
 	int error = -EINVAL;
 
-	if (buf[buflen - 1] != '\n')
+	if (buflen < 1 || buf[buflen - 1] != '\n')
 		return (-EINVAL);
 	buf[buflen - 1]= '\0';

@@ -378,7 +378,7 @@ nametoid_parse(struct cache_detail *cd, char *buf, int buflen)
 	char *buf1;
 	int error = -EINVAL;
 
-	if (buf[buflen - 1] != '\n')
+	if (buflen < 1 || buf[buflen - 1] != '\n')
 		return (-EINVAL);
 	buf[buflen - 1]= '\0';

Dan Carpenter

2012-01-18 09:54 UTC #6 lore raw

Re: [patch] nfsd: oopses in cache_parse()

Quoted 5 lines · J. Bruce Fields

Quoted 4 lines · Dan Carpenter
We fixed expkey_parse() in b2ea70afad "nfsd: Fix oops when parsing a 0 length export" but there are other cache_parse() implimentations which have the same issue.

Quoted 8 lines

Quoted 5 lines
Signed-off-by Dan Carpenter <[email protected]>
---
Since half of the implimentations get this wrong, maybe we should just check for this in cache_do_downcall(). Is there ever a valid reason to pass a zero length string to cache_parse()?
I don't think so, no. Checking in cache_do_downcall() sounds like a good idea.

Yeah. I checked and none of the cache_parse() implementations do anything with a zero length string. Ignore that patch, I'll send another.

regards,
dan carpenter

Dan Carpenter

2012-01-18 09:55 UTC #7 lore raw

PATCH 1/1 nfsd: don't allow zero length strings in cache_parse()

In reply to J. Bruce Fields

There is no point in passing a zero length string here and quite a few of that cache_parse() implementations will Oops if count is zero.

Signed-off-by Dan Carpenter <[email protected]>

net/sunrpc/cache.c +2 -0

index 465df9a..8c6598e 100644
@@ -828,6 +828,8 @@ static ssize_t cache_do_downcall(char *kaddr, const char __user *buf,
 {
 	ssize_t ret;
 
+	if (count == 0)
+		return -EINVAL;
 	if (copy_from_user(kaddr, buf, count))
 		return -EFAULT;
 	kaddr[count] = '\0';

J. Bruce Fields

2012-01-18 22:41 UTC #8 lore raw

Re: [patch] nfsd: don't allow zero length strings in cache_parse()

Quoted 3 lines · Dan Carpenter

There is no point in passing a zero length string here and quite a few of that cache_parse() implementations will Oops if count is zero.

Thanks, I'll apply.

Could we clean up any checks in cache_parse() that are unnecessary now?

--b.

Quoted 16 lines

Signed-off-by Dan Carpenter <[email protected]>

net/sunrpc/cache.c +2 -0

index 465df9a..8c6598e 100644
@@ -828,6 +828,8 @@ static ssize_t cache_do_downcall(char *kaddr, const char __user *buf,
 {
 	ssize_t ret;
 
+	if (count == 0)
+		return -EINVAL;
 	if (copy_from_user(kaddr, buf, count))
 		return -EFAULT;
 	kaddr[count] = '\0';

Dan Carpenter

2012-01-20 07:47 UTC #9 lore raw

PATCH 1/1 nfsd: remove some unneeded checks

We check for zero length strings in the caller now, so these aren't needed.

Signed-off-by Dan Carpenter <[email protected]>

fs/nfsd/export.c +1 -1

index cf8a6bd..8e9689a 100644
@@ -87,7 +87,7 @@ static int expkey_parse(struct cache_detail *cd, char *mesg, int mlen)
 	struct svc_expkey key;
 	struct svc_expkey *ek = NULL;
 
-	if (mlen < 1 || mesg[mlen-1] != '\n')
+	if (mesg[mlen - 1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;

net/sunrpc/svcauth_unix.c +1 -1

index 01153ea..6ab3573 100644
@@ -509,7 +509,7 @@ static int unix_gid_parse(struct cache_detail *cd,
 	time_t expiry;
 	struct unix_gid ug, *ugp;
 
-	if (mlen <= 0 || mesg[mlen-1] != '\n')
+	if (mesg[mlen - 1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;

J. Bruce Fields

2012-01-20 18:35 UTC #10 lore raw

Re: [patch] nfsd: remove some unneeded checks

Quoted 2 lines · Dan Carpenter

We check for zero length strings in the caller now, so these aren't needed.

Thanks! Applying.--b.

Quoted 29 lines

Signed-off-by Dan Carpenter <[email protected]>

fs/nfsd/export.c +1 -1

index cf8a6bd..8e9689a 100644
@@ -87,7 +87,7 @@ static int expkey_parse(struct cache_detail *cd, char *mesg, int mlen)
 	struct svc_expkey key;
 	struct svc_expkey *ek = NULL;
 
-	if (mlen < 1 || mesg[mlen-1] != '\n')
+	if (mesg[mlen - 1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;

net/sunrpc/svcauth_unix.c +1 -1

index 01153ea..6ab3573 100644
@@ -509,7 +509,7 @@ static int unix_gid_parse(struct cache_detail *cd,
 	time_t expiry;
 	struct unix_gid ug, *ugp;
 
-	if (mlen <= 0 || mesg[mlen-1] != '\n')
+	if (mesg[mlen - 1] != '\n')
 		return -EINVAL;
 	mesg[mlen-1] = 0;