Skip to contentSkip to conversation

nfsd: wrong index used in inner loop

PATCH In review roel 6 participants 13 messages Mar 8, 2011 – Mar 17, 2011

Subsystems (2)
NFS client NFS server
lore
Share & tools
lore

roel

author
#0 lore raw

PATCH 1/1 nfsd: wrong index used in inner loop

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

fs/nfsd/nfs4xdr.c +2 -2
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);

@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:
#1 lore raw
Quoted 2 lines · roel

Index i was already used in the outer loop

Is this as serious as it looks ?
Is it worth tho flag your own loved distro to hurry on a kernel patch ?

TIA

Quoted 34 lines
Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

fs/nfsd/nfs4xdr.c +2 -3
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);

@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:
--

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

Signature · 2 lines
J.A. Magallon <jamagallon()ono!com>     \               Software is like sex:
                                         \         It's better when it's free
#2 lore raw
In reply to roel
Quoted 8 lines · roel

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

Looks bad to me, thanks.

nfsd4_decode_create_session should probably really be broken up a little bit; if it wasn't so long this would have been more obvious.

I'll see if I can slip this into 2.6.38 with a couple other last-minute patches.... Otherwise, it'll be in 2.6.39.

--b.

Quoted 23 lines
fs/nfsd/nfs4xdr.c +2 -2
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);

@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:
#9 lore raw
In reply to roel
Quoted 23 lines · On Tue, 08 Mar 2011 22:32:26 +0100 roel <[email protected]> wrote:

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1221

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:

ooh, big bug.

I wonder why it was not previously detected at runtime. Perhaps nr_secflavs is always 1.

afacit this bug will allow a well-crafted packet to cause an infinite-until-it-oopses loop in the kernel.

#10 lore raw
Quoted 37 lines · Andrew Morton
Quoted 23 lines · On Tue, 08 Mar 2011 22:32:26 +0100 roel <[email protected]> wrote:

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1221

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:

ooh, big bug.

Quoted 2 lines

I wonder why it was not previously detected at runtime. Perhaps nr_secflavs is always 1.

Yeah, no client uses this calback security information yet.

Mi Jinlong, do you think this is something we could have caught with another pynfs test?

--b.

#11 lore raw

J. Bruce Fields:

Quoted 37 lines · Andrew Morton
Quoted 36 lines
Quoted 23 lines · On Tue, 08 Mar 2011 22:32:26 +0100 roel <[email protected]> wrote:

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1221

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:

ooh, big bug.

Quoted 7 lines
Quoted 2 lines

I wonder why it was not previously detected at runtime. Perhaps nr_secflavs is always 1.

Yeah, no client uses this calback security information yet.

Mi Jinlong, do you think this is something we could have caught with another pynfs test?

Yes, we must test it.

After testing, the following test case is OK.

--
thanks,
Mi Jinlong

Quoted 1 line

From 1afac3444b37bac66970f19c409660a304a53fb4 Mon Sep 17 00:00:00 2001

From: Mi Jinlong <[email protected]>
Date: Sun, 11 Mar 2011 09:05:22 +0800
Subject: [PATCH] CLNT: test a decode problem which use wrong index

Signed-off-by Mi Jinlong <[email protected]>
---
 nfs4.1/server41tests/st_create_session.py |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
nfs4.1/server41tests/st_create_session.py +16 -0
index ff55d10..e3a8421 100644
@@ -252,6 +252,22 @@ def testCbSecParms(t, env):
     c1 = env.c1.new_client(env.testname(t))
     sess1 = c1.create_session(sec=sec)
 
+def testCbSecParmsDec(t, env):
+    """A decode problem was found at NFS server that 
+       wrong index used in inner loop, 
+       http://marc.info/?l=linux-kernel&m=129961996327640&w=2
+
+    FLAGS: create_session all
+    CODE: CSESS16a
+    """
+    sec = [callback_sec_parms4(AUTH_NONE),
+           callback_sec_parms4(RPCSEC_GSS, cbsp_gss_handles=gss_cb_handles4(RPC_GSS_SVC_PRIVACY, "Handle from server", "Client handle")),
+           callback_sec_parms4(AUTH_SYS, cbsp_sys_cred=authsys_parms(5, "Random machine name", 7, 11, [])),
+           ]
+                               
+    c1 = env.c1.new_client(env.testname(t))
+    sess1 = c1.create_session(sec=sec)
+
 def testRdmaArray0(t, env):
     """Test 0 length rdma arrays
Signature · 1 line
1.7.4.1
#3 lore raw
In reply to J. Bruce Fields

J. Bruce Fields:

Quoted 32 lines · roel
Quoted 8 lines

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

Looks bad to me, thanks.

nfsd4_decode_create_session should probably really be broken up a little bit; if it wasn't so long this would have been more obvious.

I'll see if I can slip this into 2.6.38 with a couple other last-minute patches.... Otherwise, it'll be in 2.6.39.

--b.

Quoted 12 lines
fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
Quoted 8 lines

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1219

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
 	READ_BUF(16);

@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);

We must not use dummy for index here.
After the first index, READ32(dummy) will change dummy!!!!

The following patch fix this problem.

--
thanks,
Mi Jinlong
============================================================

We must not use dummy for index.
After the first index, READ32(dummy) will change dummy!!!!

Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/nfsd/nfs4xdr.c +2 -2
index 615f0a9..8dd70d0 100644
@@ -1140,7 +1140,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 {
 	DECODE_HEAD;
 
-	u32 dummy;
+	u32 dummy, tmp;
 	char *machine_name;
 	int i, j;
 	int nr_secflavs;

@@ -1216,7 +1216,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ32(dummy);
 			READ_BUF(dummy * 4);
 			for (j = 0; j < dummy; ++j)
-				READ32(dummy);
+				READ32(tmp);
 			break;
 		case RPC_AUTH_GSS:
 			dprintk("RPC_AUTH_GSS callback secflavor "
Signature · 1 line
1.7.4.1
#12 lore raw
In reply to Mi Jinlong
Quoted 43 lines · Mi Jinlong

J. Bruce Fields:

Quoted 40 lines · Andrew Morton
Quoted 38 lines
Quoted 23 lines · On Tue, 08 Mar 2011 22:32:26 +0100 roel <[email protected]> wrote:

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
 
 	READ_BUF(16);

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1221

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:

ooh, big bug.

I wonder why it was not previously detected at runtime. Perhaps nr_secflavs is always 1.

Quoted 8 lines
Quoted 4 lines

Yeah, no client uses this calback security information yet.

Mi Jinlong, do you think this is something we could have caught with another pynfs test?

Yes, we must test it.

After testing, the following test case is OK.

I've pushed out a tree with your additional tests to

	git://linux-nfs.org/~bfields/pynfs41.git

Let me know what I'm missing.

Thanks again.--b.

Quoted 47 lines

--
thanks,
Mi Jinlong

Quoted 1 line

From 1afac3444b37bac66970f19c409660a304a53fb4 Mon Sep 17 00:00:00 2001

From: Mi Jinlong <[email protected]>
Date: Sun, 11 Mar 2011 09:05:22 +0800
Subject: [PATCH] CLNT: test a decode problem which use wrong index

Signed-off-by Mi Jinlong <[email protected]>
---
 nfs4.1/server41tests/st_create_session.py |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
nfs4.1/server41tests/st_create_session.py +16 -0
index ff55d10..e3a8421 100644
@@ -252,6 +252,22 @@ def testCbSecParms(t, env):
     c1 = env.c1.new_client(env.testname(t))
     sess1 = c1.create_session(sec=sec)
 
+def testCbSecParmsDec(t, env):
+    """A decode problem was found at NFS server that 
+       wrong index used in inner loop, 
+       http://marc.info/?l=linux-kernel&m=129961996327640&w=2
+
+    FLAGS: create_session all
+    CODE: CSESS16a
+    """
+    sec = [callback_sec_parms4(AUTH_NONE),
+           callback_sec_parms4(RPCSEC_GSS, cbsp_gss_handles=gss_cb_handles4(RPC_GSS_SVC_PRIVACY, "Handle from server", "Client handle")),
+           callback_sec_parms4(AUTH_SYS, cbsp_sys_cred=authsys_parms(5, "Random machine name", 7, 11, [])),
+           ]
+                               
+    c1 = env.c1.new_client(env.testname(t))
+    sess1 = c1.create_session(sec=sec)
+
 def testRdmaArray0(t, env):
     """Test 0 length rdma arrays
Signature · 1 line
1.7.4.1
#4 lore raw
In reply to Mi Jinlong
Quoted 35 lines · Mi Jinlong

J. Bruce Fields:

Quoted 32 lines · roel
Quoted 8 lines

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

Looks bad to me, thanks.

nfsd4_decode_create_session should probably really be broken up a little bit; if it wasn't so long this would have been more obvious.

I'll see if I can slip this into 2.6.38 with a couple other last-minute patches.... Otherwise, it'll be in 2.6.39.

--b.

Quoted 12 lines
fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
Quoted 11 lines
Quoted 8 lines

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1219

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
 	READ_BUF(16);

@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);

We must not use dummy for index here.
After the first index, READ32(dummy) will change dummy!!!!

Actually, wait, this is kind of silly. I don't see why we couldn't just skip the loop and do

	p += dummy;

Also, your new test is still failing with a BAD_XDR error. Well, maybe the test should fail--we don't really implement this yet anyway--but it should at least be getting past the xdr decoding. So something else is still wrong.

--b.

Quoted 42 lines

The following patch fix this problem.

--
thanks,
Mi Jinlong
============================================================

We must not use dummy for index.
After the first index, READ32(dummy) will change dummy!!!!

Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/nfsd/nfs4xdr.c +2 -2
index 615f0a9..8dd70d0 100644
@@ -1140,7 +1140,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 {
 	DECODE_HEAD;
 
-	u32 dummy;
+	u32 dummy, tmp;
 	char *machine_name;
 	int i, j;
 	int nr_secflavs;

@@ -1216,7 +1216,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ32(dummy);
 			READ_BUF(dummy * 4);
 			for (j = 0; j < dummy; ++j)
-				READ32(dummy);
+				READ32(tmp);
 			break;
 		case RPC_AUTH_GSS:
 			dprintk("RPC_AUTH_GSS callback secflavor "
Signature · 1 line
1.7.4.1
#5 lore raw
Quoted 48 lines · J. Bruce Fields
Quoted 35 lines · Mi Jinlong

J. Bruce Fields:

Quoted 32 lines · roel
Quoted 8 lines

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

Looks bad to me, thanks.

nfsd4_decode_create_session should probably really be broken up a little bit; if it wasn't so long this would have been more obvious.

I'll see if I can slip this into 2.6.38 with a couple other last-minute patches.... Otherwise, it'll be in 2.6.39.

--b.

Quoted 12 lines
fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
Quoted 11 lines
Quoted 8 lines

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1219

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
 	READ_BUF(16);

@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);

We must not use dummy for index here.
After the first index, READ32(dummy) will change dummy!!!!

Quoted 4 lines

Actually, wait, this is kind of silly. I don't see why we couldn't just skip the loop and do

	p += dummy;

This is exactly why I _hate_ the READ*() macros and their ilk, and am really happy we got rid of them in the client.

READ_BUF() _sets_ p to whatever the value of argp->p is, and then updates argp->p. It is just very very very hard to see that due to the lack of transparency.

IOW: You don't need the "p += dummy" either. That happens automatically when you next invoke READ_BUF().

Trond

Signature · 6 lines
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com
#7 lore raw
In reply to J. Bruce Fields

J. Bruce Fields:

Quoted 48 lines · Mi Jinlong
Quoted 33 lines

J. Bruce Fields:

Quoted 31 lines · roel
Quoted 8 lines

Index i was already used in the outer loop

Signed-off-by Roel Kluin <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Not 100% sure this one is needed but it looks suspicious.

Looks bad to me, thanks.

nfsd4_decode_create_session should probably really be broken up a little bit; if it wasn't so long this would have been more obvious.

I'll see if I can slip this into 2.6.38 with a couple other last-minute patches.... Otherwise, it'll be in 2.6.39.

--b.

Quoted 12 lines
fs/nfsd/nfs4xdr.c +1 -1
index 1275b86..615f0a9 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i;
+	int i, j;
 	int nr_secflavs;
Quoted 10 lines
Quoted 8 lines

fs/nfsd/nfs4xdr.c nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1219

Quoted 8 lines
fs/nfsd/nfs4xdr.c +1 -1
 	READ_BUF(16);

@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);

We must not use dummy for index here.
After the first index, READ32(dummy) will change dummy!!!!

Actually, wait, this is kind of silly. I don't see why we couldn't just skip the loop and do

Quoted 6 lines
	p += dummy;

Also, your new test is still failing with a BAD_XDR error. Well, maybe the test should fail--we don't really implement this yet anyway--but it should at least be getting past the xdr decoding. So something else is still wrong.

How did you modify it??

When testing it, I modify as

- for (j = 0; j < dummy; ++j)
- READ32(dummy);
+ p += dummy;

or

- for (j = 0; j < dummy; ++j)
- READ32(dummy);

Test case CSESS16 and CSESS16a are PASS,
I can't get BAD_XDR error as you said.

--
thanks,
Mi Jinlong

Quoted 49 lines

--b.

Quoted 41 lines

The following patch fix this problem.

--
thanks,
Mi Jinlong
============================================================

We must not use dummy for index.
After the first index, READ32(dummy) will change dummy!!!!

Signed-off-by Mi Jinlong <[email protected]>
---
 fs/nfsd/nfs4xdr.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
fs/nfsd/nfs4xdr.c +2 -2
index 615f0a9..8dd70d0 100644
@@ -1140,7 +1140,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 {
 	DECODE_HEAD;
 
-	u32 dummy;
+	u32 dummy, tmp;
 	char *machine_name;
 	int i, j;
 	int nr_secflavs;

@@ -1216,7 +1216,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ32(dummy);
 			READ_BUF(dummy * 4);
 			for (j = 0; j < dummy; ++j)
-				READ32(dummy);
+				READ32(tmp);
 			break;
 		case RPC_AUTH_GSS:
 			dprintk("RPC_AUTH_GSS callback secflavor "
Signature · 1 line
1.7.4.1

--
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

Signature · 3 lines
----
thanks
Mi Jinlong
#6 lore raw
In reply to Trond Myklebust
Quoted 17 lines · Trond Myklebust
Quoted 16 lines · J. Bruce Fields
Quoted 14 lines · Mi Jinlong

J. Bruce Fields:

Quoted 8 lines · roel

Review nfsd4_decode_create_session(struct nfsd4_compoundargs *argp, · line 1219

Quoted 7 lines
(quoted diff) +1 -1
@@ -1215,7 +1215,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (i = 0; i < dummy; ++i)
+			for (j = 0; j < dummy; ++j)
 				READ32(dummy);

We must not use dummy for index here.
After the first index, READ32(dummy) will change dummy!!!!

Quoted 7 lines
Quoted 4 lines

Actually, wait, this is kind of silly. I don't see why we couldn't just skip the loop and do

	p += dummy;

This is exactly why I _hate_ the READ*() macros and their ilk, and am really happy we got rid of them in the client.

Agreed, I'm all for getting rid of them completely.

Quoted 6 lines

READ_BUF() _sets_ p to whatever the value of argp->p is, and then updates argp->p. It is just very very very hard to see that due to the lack of transparency.

IOW: You don't need the "p += dummy" either. That happens automatically when you next invoke READ_BUF().

Yes, you're right, we could remove that silly loop entirely.

--b.

#8 lore raw
In reply to Mi Jinlong
Quoted 21 lines · Mi Jinlong

J. Bruce Fields:

Quoted 9 lines

Actually, wait, this is kind of silly. I don't see why we couldn't just skip the loop and do

	p += dummy;

Also, your new test is still failing with a BAD_XDR error. Well, maybe the test should fail--we don't really implement this yet anyway--but it should at least be getting past the xdr decoding. So something else is still wrong.

How did you modify it??

When testing it, I modify as

- for (j = 0; j < dummy; ++j)
- READ32(dummy);
+ p += dummy;

Quoted 7 lines

or

- for (j = 0; j < dummy; ++j)
- READ32(dummy);

Test case CSESS16 and CSESS16a are PASS,
I can't get BAD_XDR error as you said.

Yes, I thought I had the former, but perhaps I had the wrong kernel running on my test server. I've confirmed those tests pass after the following patch.

--b.

commit 5a02ab7c3c4580f94d13c683721039855b67cda6

Author: Mi Jinlong <[email protected]>
Date:   Fri Mar 11 12:13:55 2011 +0800

nfsd: wrong index used in inner loop

We must not use dummy for index.
After the first index, READ32(dummy) will change dummy!!!!

Signed-off-by Mi Jinlong <[email protected]>
[[email protected]: Trond points out READ_BUF alone is sufficient.]
Cc [email protected]
Signed-off-by J. Bruce Fields <[email protected]>
fs/nfsd/nfs4xdr.c +1 -4
index 615f0a9..c6766af 100644
@@ -1142,7 +1142,7 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 
 	u32 dummy;
 	char *machine_name;
-	int i, j;
+	int i;
 	int nr_secflavs;
 
 	READ_BUF(16);

@@ -1215,8 +1215,6 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy * 4);
-			for (j = 0; j < dummy; ++j)
-				READ32(dummy);
 			break;
 		case RPC_AUTH_GSS:
 			dprintk("RPC_AUTH_GSS callback secflavor "

@@ -1232,7 +1230,6 @@ nfsd4_decode_create_session(struct nfsd4_compoundargs *argp,
 			READ_BUF(4);
 			READ32(dummy);
 			READ_BUF(dummy);
-			p += XDR_QUADLEN(dummy);
 			break;
 		default:
 			dprintk("Illegal callback secflavor\n");