Bug in xdr_copy_to_scratch???

Hi Trond,

I'm currently trying to track down the cause of some very
odd behaviour in readdir in openSUSE 11.4 (2.6.37.3 based).

I think it might be caused by xdr_copy_to_scratch not quite behaving correctly.

In particular, when it has to copy into the scratch buffer it only copies 'nbytes' bytes - which sounds reasonable but isn't. It should copy XDR_QUADLEN(nbytes) words.

In particular, nfs3_decode_dirent contains:

	p = xdr_inline_decode(xdr, entry->len + 8);
	if (unlikely(!p))
		goto out_overflow;
	entry->name = (const char *) p;
	p += XDR_QUADLEN(entry->len);
	entry->prev_cookie = entry->cookie;
	p = xdr_decode_hyper(p, &entry->cookie);

where the cookie needs all of those last few bytes which we would only get by rounding nbytes up to a multiple of 4.

I haven't developed or tested a fix yet, but as it is clearly a bug, I thought I would let you know before I call it a night.

Quoted 23 lines · NeilBrown

Hi Trond,

I'm currently trying to track down the cause of some very
odd behaviour in readdir in openSUSE 11.4 (2.6.37.3 based).

I think it might be caused by xdr_copy_to_scratch not quite behaving correctly.

In particular, when it has to copy into the scratch buffer it only copies 'nbytes' bytes - which sounds reasonable but isn't. It should copy XDR_QUADLEN(nbytes) words.

In particular, nfs3_decode_dirent contains:

	p = xdr_inline_decode(xdr, entry->len + 8);
	if (unlikely(!p))
		goto out_overflow;
	entry->name = (const char *) p;
	p += XDR_QUADLEN(entry->len);
	entry->prev_cookie = entry->cookie;
	p = xdr_decode_hyper(p, &entry->cookie);

Quoted 6 lines

where the cookie needs all of those last few bytes which we would only get by rounding nbytes up to a multiple of 4.

I haven't developed or tested a fix yet, but as it is clearly a bug, I thought I would let you know before I call it a night.

Hi Neil,

I'm not sure I 100% agree that is a bug in xdr_copy_to_scratch(). I think it is rather an artifact of the fact that xdr_inline_decode takes a byte argument instead of a word argument, which means that combining buffer lengths needs to done with care: my fault :-(.

To illustrate what I mean, consider the following snippet of xdr:

	p = xdr_decode_inline(xdr, len1 + len2);
	name1 = p;
	p += XDR_QUADLEN(len1);
	name2 = p;

No matter what we do in xdr_decode_inline(), there is no way we can determine the true value of XDR_QUADLEN(len1)<<2 + XDR_QUADLEN(len2)<<2 if len1 and len2 are arbitrary buffer lengths.

So I'd argue that while we could fix this issue in xdr_copy_to_scratch for this particular case, in reality nfs3_decode_dirent should not be asking for a buffer of (entry->len + 8) bytes when it knows there is an alignment issue due to the fact that the 8 cookie bytes follows the string.

Quoted 49 lines · On Wed, 16 Mar 2011 09:42:54 -0400 Trond Myklebust…

Quoted 23 lines · NeilBrown

Hi Trond,

I'm currently trying to track down the cause of some very
odd behaviour in readdir in openSUSE 11.4 (2.6.37.3 based).

I think it might be caused by xdr_copy_to_scratch not quite behaving correctly.

In particular, when it has to copy into the scratch buffer it only copies 'nbytes' bytes - which sounds reasonable but isn't. It should copy XDR_QUADLEN(nbytes) words.

In particular, nfs3_decode_dirent contains:

	p = xdr_inline_decode(xdr, entry->len + 8);
	if (unlikely(!p))
		goto out_overflow;
	entry->name = (const char *) p;
	p += XDR_QUADLEN(entry->len);
	entry->prev_cookie = entry->cookie;
	p = xdr_decode_hyper(p, &entry->cookie);

Quoted 6 lines

where the cookie needs all of those last few bytes which we would only get by rounding nbytes up to a multiple of 4.

I haven't developed or tested a fix yet, but as it is clearly a bug, I thought I would let you know before I call it a night.

Hi Neil,

I'm not sure I 100% agree that is a bug in xdr_copy_to_scratch(). I think it is rather an artifact of the fact that xdr_inline_decode takes a byte argument instead of a word argument, which means that combining buffer lengths needs to done with care: my fault :-(.

To illustrate what I mean, consider the following snippet of xdr:

	p = xdr_decode_inline(xdr, len1 + len2);
	name1 = p;
	p += XDR_QUADLEN(len1);
	name2 = p;

No matter what we do in xdr_decode_inline(), there is no way we can determine the true value of XDR_QUADLEN(len1)<<2 + XDR_QUADLEN(len2)<<2 if len1 and len2 are arbitrary buffer lengths.

Quoted 5 lines

So I'd argue that while we could fix this issue in xdr_copy_to_scratch for this particular case, in reality nfs3_decode_dirent should not be asking for a buffer of (entry->len + 8) bytes when it knows there is an alignment issue due to the fact that the 8 cookie bytes follows the string.

Your argument certainly has some merit.
Probably the question to ask is "what sort of fix is most likely to avoid such problems recurring in the future", and I cannot convince myself that either is clearly better than the other on that basis.

I note that in 2.6.38 the readdir problem has been 'fixed' by the introduction of decode_inline_filename3 and similar functions. However there is still one place where xdr_decode_inline is used in an unsafe way - see patch below.

We should probably submit a fix to 2.6.37-stable though. For that it is possibly simplest to tell xdr_decode_inline to round nbytes up to a multiple of 4 - would you agree?

Also: looking at xdr_copy_to_scratch again, is there any chance that 'cplen' could not be a multiple of 4? I think not but I'm not 100% sure. If it were not a multiple of 4, then the __xdr_inline_decode call would do the wrong thing.

And finally - it bothers me a little bit that a call to xdr_copy_to_scratch will corrupt the value returned by the previous call to xdr_copy_to_scratch. This is unlikely to be a problem as you get at most one of those calls every 4096 bytes, and the previous value will almost certainly have been used before the next value is copied over it. But it isn't clear what guarantees that it will have been used, so a future change or extension to the protocol could conceivably result in corruption happening in the scratch buffer. Maybe it isn't worth worrying about.... not sure.

Fix theoretical decoding problem in nfsv4 compound header decoding.

As all XDR encoded fields are rounded up to a multiple of 4 bytes in size, a string field followed by a number field could have padding between the string and number.

If xdr_inline_decode is asked to decode the combined field (by summing the lengths) it will normally work. However if it needs to copy the field into that 'scratch' space to avoid a break between pages, it will only copy the requested number of bytes, as it assumes any padding is at the end - so the number field will not be fully copied.

So in decode_compound_hdr, call xdr_inline_decode twice instead of risking an incorrect decode.

As this field is always near the start of a packet, the chance that it will ever cross a page boundary and so cause a problem is vanishingly small, but having 'wrong' code could set an example that might get copied. So fix it anyway.

Signed-off-by NeilBrown <[email protected]>

fs/nfs/nfs4xdr.c +1 -1

index 4e2c168..c52b50c 100644
@@ -2638,11 +2638,13 @@ static int decode_compound_hdr(struct xdr_stream *xdr, struct compound_hdr *hdr)
 	hdr->status = be32_to_cpup(p++);
 	hdr->taglen = be32_to_cpup(p);
 
-	p = xdr_inline_decode(xdr, hdr->taglen + 4);
+	p = xdr_inline_decode(xdr, hdr->taglen);

We should probably submit a fix to 2.6.37-stable though. For that it is possibly simplest to tell xdr_decode_inline to round nbytes up to a multiple of 4 - would you agree?

Quoted 3 lines · NeilBrown

We should probably submit a fix to 2.6.37-stable though. For that it is possibly simplest to tell xdr_decode_inline to round nbytes up to a multiple of 4 - would you agree?

How about the following fix for 2.6.37 stable?

Quoted 6 lines · On Thu, 17 Mar 2011 14:01:05 -0400 Trond Myklebust…

Quoted 3 lines · NeilBrown

We should probably submit a fix to 2.6.37-stable though. For that it is possibly simplest to tell xdr_decode_inline to round nbytes up to a multiple of 4 - would you agree?

How about the following fix for 2.6.37 stable?

That is good for NFSv3, but NFSv2 has the same problem. Code fragment is

	p = xdr_inline_decode(xdr, entry->len + 4);
	if (unlikely(!p))
		goto out_overflow;
	entry->name	  = (const char *) p;
	p		 += XDR_QUADLEN(entry->len);
	entry->prev_cookie	  = entry->cookie;
	entry->cookie	  = ntohl(*p++);

so again we have the cookie after the name and they are decoded together.

Quoted 19 lines · NeilBrown

Quoted 6 lines · On Thu, 17 Mar 2011 14:01:05 -0400 Trond Myklebust…

Quoted 3 lines · NeilBrown

We should probably submit a fix to 2.6.37-stable though. For that it is possibly simplest to tell xdr_decode_inline to round nbytes up to a multiple of 4 - would you agree?

How about the following fix for 2.6.37 stable?

That is good for NFSv3, but NFSv2 has the same problem. Code fragment is

	p = xdr_inline_decode(xdr, entry->len + 4);
	if (unlikely(!p))
		goto out_overflow;
	entry->name	  = (const char *) p;
	p		 += XDR_QUADLEN(entry->len);
	entry->prev_cookie	  = entry->cookie;
	entry->cookie	  = ntohl(*p++);

Quoted 1 line

so again we have the cookie after the name and they are decoded together.

Fair enough. I'll fix that one too.

Quoted 22 lines · Trond Myklebust

Quoted 20 lines · NeilBrown

Quoted 6 lines · On Thu, 17 Mar 2011 14:01:05 -0400 Trond Myklebust…

Quoted 3 lines · NeilBrown

We should probably submit a fix to 2.6.37-stable though. For that it is possibly simplest to tell xdr_decode_inline to round nbytes up to a multiple of 4 - would you agree?

How about the following fix for 2.6.37 stable?

That is good for NFSv3, but NFSv2 has the same problem. Code fragment is

	p = xdr_inline_decode(xdr, entry->len + 4);
	if (unlikely(!p))
		goto out_overflow;
	entry->name	  = (const char *) p;
	p		 += XDR_QUADLEN(entry->len);
	entry->prev_cookie	  = entry->cookie;
	entry->cookie	  = ntohl(*p++);

so again we have the cookie after the name and they are decoded together.

Quoted 2 lines

Fair enough. I'll fix that one too.

Here is the result.

BTW, those are the only cases we care about in -stable. There are no other callers of xdr_set_scratch_buffer().

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

Quoted 1 line

From f71825a9002d4008a5b2ac947bc03b9d7c788557 Mon Sep 17 00:00:00 2001

From: Trond Myklebust <[email protected]>
Date: Thu, 17 Mar 2011 13:42:31 -0400
Subject: [PATCH] NFS: Fix a decoding problem in nfs3_decode_dirent

When we decode a filename followed by an 8-byte cookie, we need to consider the fact that the filename and cookie are 32-bit word aligned. Presently, we may end up copying insufficient amounts of data when xdr_inline_decode() needs to invoke xdr_copy_to_scratch to deal with a page boundary.

The following patch fixes the issue by first decoding the filename, and then decoding the cookie.

Reported-by Neil Brown <[email protected]>

Signed-off-by Trond Myklebust <[email protected]>

---
 fs/nfs/nfs2xdr.c |    6 ++++--
 fs/nfs/nfs3xdr.c |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

fs/nfs/nfs2xdr.c +4 -2

index b382a1b..33a038d 100644
@@ -477,11 +477,13 @@ nfs_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry, struct nfs_se
 	entry->ino	  = ntohl(*p++);
 	entry->len	  = ntohl(*p++);
 
-	p = xdr_inline_decode(xdr, entry->len + 4);
+	p = xdr_inline_decode(xdr, entry->len);
 	if (unlikely(!p))
 		goto out_overflow;
 	entry->name	  = (const char *) p;
-	p		 += XDR_QUADLEN(entry->len);
+	p = xdr_inline_decode(xdr, 4);
+	if (unlikely(!p))
+		goto out_overflow;
 	entry->prev_cookie	  = entry->cookie;
 	entry->cookie	  = ntohl(*p++);
 

fs/nfs/nfs3xdr.c +4 -2

index ba91236..dcd934f 100644
@@ -614,11 +614,13 @@ nfs3_decode_dirent(struct xdr_stream *xdr, struct nfs_entry *entry, struct nfs_s
 	p = xdr_decode_hyper(p, &entry->ino);
 	entry->len  = ntohl(*p++);
 
-	p = xdr_inline_decode(xdr, entry->len + 8);
+	p = xdr_inline_decode(xdr, entry->len);
 	if (unlikely(!p))
 		goto out_overflow;
 	entry->name = (const char *) p;
-	p += XDR_QUADLEN(entry->len);
+	p = xdr_inline_decode(xdr, 8);
+	if (unlikely(!p))
+		goto out_overflow;
 	entry->prev_cookie = entry->cookie;
 	p = xdr_decode_hyper(p, &entry->cookie);