[PATCH] NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

[PATCH] NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

[David Howells]

Check the bounds of length specifiers more thoroughly in the XDR decoding of
NFS4 readdir reply data.

Currently, if the server returns a bitmap or attr length that causes the
current decode point pointer to wrap, this could go undetected (consider a
small "negative" length on a 32-bit machine).

Also add a check into the main XDR decode handler to make sure that the amount
of data is a multiple of four bytes (as specified by RFC-1014).  This makes
sure that we can do u32* pointer subtraction in the NFS client without risking
an undefined result (the result is undefined if the pointers are not correctly
aligned with respect to one another).

Signed-Off-By: David Howells <dhowells@redhat.com>
---

 fs/nfs/nfs4xdr.c  |   22 +++++++++++-----------
 net/sunrpc/clnt.c |   11 +++++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 992a713..41a295a 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3355,7 +3355,7 @@ static int decode_readdir(struct xdr_str
 	struct kvec	*iov = rcvbuf->head;
 	unsigned int	nr, pglen = rcvbuf->page_len;
 	uint32_t	*end, *entry, *p, *kaddr;
-	uint32_t	len, attrlen;
+	uint32_t	len, attrlen, xlen;
 	int 		hdrlen, recvd, status;
 
 	status = decode_op_hdr(xdr, OP_READDIR);
@@ -3374,13 +3374,12 @@ static int decode_readdir(struct xdr_str
 	if (pglen > recvd)
 		pglen = recvd;
 	xdr_read_pages(xdr, pglen);
-
 	BUG_ON(pglen + readdir->pgbase > PAGE_CACHE_SIZE);
 	kaddr = p = (uint32_t *) kmap_atomic(page, KM_USER0);
-	end = (uint32_t *) ((char *)p + pglen + readdir->pgbase);
+	end = p + ((pglen + readdir->pgbase) >> 2);
 	entry = p;
 	for (nr = 0; *p++; nr++) {
-		if (p + 3 > end)
+		if (end - p < 3)
 			goto short_pkt;
 		dprintk("cookie = %Lu, ", *((unsigned long long *)p));
 		p += 2;			/* cookie */
@@ -3389,18 +3388,19 @@ static int decode_readdir(struct xdr_str
 			printk(KERN_WARNING "NFS: giant filename in readdir (len 0x%x)\n", len);
 			goto err_unmap;
 		}
-		dprintk("filename = %*s\n", len, (char *)p);
-		p += XDR_QUADLEN(len);
-		if (p + 1 > end)
+		xlen = XDR_QUADLEN(len);
+		if (end - p < xlen)
 			goto short_pkt;
+		dprintk("filename = %*s\n", len, (char *)p);
+		p += xlen;
 		len = ntohl(*p++);	/* bitmap length */
-		p += len;
-		if (p + 1 > end)
+		if (end - p < len)
 			goto short_pkt;
+		p += len;
 		attrlen = XDR_QUADLEN(ntohl(*p++));
-		p += attrlen;		/* attributes */
-		if (p + 2 > end)
+		if (end - p < attrlen + 1)
 			goto short_pkt;
+		p += attrlen;		/* attributes */
 		entry = p;
 	}
 	if (!nr && (entry[0] != 0 || entry[1] == 0))
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index d9eac70..3e19d32 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1181,6 +1181,17 @@ call_verify(struct rpc_task *task)
 	u32	*p = iov->iov_base, n;
 	int error = -EACCES;
 
+	if ((task->tk_rqstp->rq_rcv_buf.len & 3) != 0) {
+		/* RFC-1014 says that the representation of XDR data must be a
+		 * multiple of four bytes
+		 * - if it isn't pointer subtraction in the NFS client may give
+		 *   undefined results
+		 */
+		printk(KERN_WARNING
+		       "call_verify: XDR representation not a multiple of"
+		       " 4 bytes: 0x%x\n", task->tk_rqstp->rq_rcv_buf.len);
+		goto out_eio;
+	}
 	if ((len -= 3) < 0)
 		goto out_overflow;
 	p += 1;	/* skip XID */

Re: [PATCH] NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

On Tue, 2006-08-22 at 17:19 +0100, David Howells wrote:
> Check the bounds of length specifiers more thoroughly in the XDR decoding of
> NFS4 readdir reply data.

Hmm... Your patch fails to check for buffer overflows on the read of the
bitmap/attribute length, and on the end-of-record markers. The attached
slightly revamped patch corrects those oversights.

Cheers,
  Trond

[-- Attachment #2: linux-2.6.18-069-check_lengths_in_nfs4_readdir_xdr_decode.dif --]
[-- Type: message/rfc822, Size: 3573 bytes --]

From: David Howells <dhowells@redhat.com>
Subject: No Subject
Date: Thu, 24 Aug 2006 11:20:51 -0400
Message-ID: <1156432851.5629.23.camel@localhost>

Check the bounds of length specifiers more thoroughly in the XDR decoding of
NFS4 readdir reply data.

Currently, if the server returns a bitmap or attr length that causes the
current decode point pointer to wrap, this could go undetected (consider a
small "negative" length on a 32-bit machine).

Also add a check into the main XDR decode handler to make sure that the amount
of data is a multiple of four bytes (as specified by RFC-1014).  This makes
sure that we can do u32* pointer subtraction in the NFS client without risking
an undefined result (the result is undefined if the pointers are not correctly
aligned with respect to one another).

Signed-Off-By: David Howells <dhowells@redhat.com>
Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/nfs/nfs4xdr.c  |   21 +++++++++++----------
 net/sunrpc/clnt.c |   11 +++++++++++
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 83c90d0..3dd413f 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -3355,7 +3355,7 @@ static int decode_readdir(struct xdr_str
 	struct kvec	*iov = rcvbuf->head;
 	unsigned int	nr, pglen = rcvbuf->page_len;
 	uint32_t	*end, *entry, *p, *kaddr;
-	uint32_t	len, attrlen;
+	uint32_t	len, attrlen, xlen;
 	int 		hdrlen, recvd, status;
 
 	status = decode_op_hdr(xdr, OP_READDIR);
@@ -3377,10 +3377,10 @@ static int decode_readdir(struct xdr_str
 
 	BUG_ON(pglen + readdir->pgbase > PAGE_CACHE_SIZE);
 	kaddr = p = (uint32_t *) kmap_atomic(page, KM_USER0);
-	end = (uint32_t *) ((char *)p + pglen + readdir->pgbase);
+	end = p + ((pglen + readdir->pgbase) >> 2);
 	entry = p;
 	for (nr = 0; *p++; nr++) {
-		if (p + 3 > end)
+		if (end - p < 3)
 			goto short_pkt;
 		dprintk("cookie = %Lu, ", *((unsigned long long *)p));
 		p += 2;			/* cookie */
@@ -3389,18 +3389,19 @@ static int decode_readdir(struct xdr_str
 			printk(KERN_WARNING "NFS: giant filename in readdir (len 0x%x)\n", len);
 			goto err_unmap;
 		}
-		dprintk("filename = %*s\n", len, (char *)p);
-		p += XDR_QUADLEN(len);
-		if (p + 1 > end)
+		xlen = XDR_QUADLEN(len);
+		if (end - p < xlen + 1)
 			goto short_pkt;
+		dprintk("filename = %*s\n", len, (char *)p);
+		p += xlen;
 		len = ntohl(*p++);	/* bitmap length */
-		p += len;
-		if (p + 1 > end)
+		if (end - p < len + 1)
 			goto short_pkt;
+		p += len;
 		attrlen = XDR_QUADLEN(ntohl(*p++));
-		p += attrlen;		/* attributes */
-		if (p + 2 > end)
+		if (end - p < attrlen + 2)
 			goto short_pkt;
+		p += attrlen;		/* attributes */
 		entry = p;
 	}
 	if (!nr && (entry[0] != 0 || entry[1] == 0))
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 88c98ac..87efcd2 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1229,6 +1229,17 @@ call_verify(struct rpc_task *task)
 	u32	*p = iov->iov_base, n;
 	int error = -EACCES;
 
+	if ((task->tk_rqstp->rq_rcv_buf.len & 3) != 0) {
+		/* RFC-1014 says that the representation of XDR data must be a
+		 * multiple of four bytes
+		 * - if it isn't pointer subtraction in the NFS client may give
+		 *   undefined results
+		 */
+		printk(KERN_WARNING
+		       "call_verify: XDR representation not a multiple of"
+		       " 4 bytes: 0x%x\n", task->tk_rqstp->rq_rcv_buf.len);
+		goto out_eio;
+	}
 	if ((len -= 3) < 0)
 		goto out_overflow;
 	p += 1;	/* skip XID */

Re: [PATCH] NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

[David Howells]

So, what you've done is:

-+		if (end - p < xlen)
++		if (end - p < xlen + 1)
  			goto short_pkt;
 +		dprintk("filename = %*s\n", len, (char *)p);
 +		p += xlen;
  		len = ntohl(*p++);	/* bitmap length */
 -		p += len;
 -		if (p + 1 > end)
-+		if (end - p < len)
++		if (end - p < len + 1)
  			goto short_pkt;
 +		p += len;
  		attrlen = XDR_QUADLEN(ntohl(*p++));
 -		p += attrlen;		/* attributes */
 -		if (p + 2 > end)
-+		if (end - p < attrlen + 1)
++		if (end - p < attrlen + 2)

But is this equivalent:

-+		if (end - p < xlen)
++		if (end - p <= xlen)
  			goto short_pkt;
 +		dprintk("filename = %*s\n", len, (char *)p);
 +		p += xlen;
  		len = ntohl(*p++);	/* bitmap length */
 -		p += len;
 -		if (p + 1 > end)
-+		if (end - p < len)
++		if (end - p <= len)
  			goto short_pkt;
 +		p += len;
  		attrlen = XDR_QUADLEN(ntohl(*p++));
 -		p += attrlen;		/* attributes */
 -		if (p + 2 > end)
-+		if (end - p < attrlen + 1)
++		if (end - p <= attrlen + 1)

Do you think?

David

Re: [PATCH] NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

[Trond Myklebust]

On Thu, 2006-08-24 at 19:35 +0100, David Howells wrote:
> So, what you've done is:
> 
> -+		if (end - p < xlen)
> ++		if (end - p < xlen + 1)
>   			goto short_pkt;
>  +		dprintk("filename = %*s\n", len, (char *)p);
>  +		p += xlen;
>   		len = ntohl(*p++);	/* bitmap length */
>  -		p += len;
>  -		if (p + 1 > end)
> -+		if (end - p < len)
> ++		if (end - p < len + 1)
>   			goto short_pkt;
>  +		p += len;
>   		attrlen = XDR_QUADLEN(ntohl(*p++));
>  -		p += attrlen;		/* attributes */
>  -		if (p + 2 > end)
> -+		if (end - p < attrlen + 1)
> ++		if (end - p < attrlen + 2)
> 
> But is this equivalent:
> 
> -+		if (end - p < xlen)
> ++		if (end - p <= xlen)
>   			goto short_pkt;
>  +		dprintk("filename = %*s\n", len, (char *)p);
>  +		p += xlen;
>   		len = ntohl(*p++);	/* bitmap length */
>  -		p += len;
>  -		if (p + 1 > end)
> -+		if (end - p < len)
> ++		if (end - p <= len)
>   			goto short_pkt;
>  +		p += len;
>   		attrlen = XDR_QUADLEN(ntohl(*p++));
>  -		p += attrlen;		/* attributes */
>  -		if (p + 2 > end)
> -+		if (end - p < attrlen + 1)
> ++		if (end - p <= attrlen + 1)
                              ^^^^^^^^^^^^^^
> 
> Do you think?

No. I find that mixture of < and <= is much less easy to read. Besides,
the compiler should be able to optimise that for me.

Cheers,
  Trond

Re: [PATCH] NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

[David Howells]

Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> No. I find that mixture of < and <= is much less easy to read. Besides,
> the compiler should be able to optimise that for me.

So you don't think they're mathematically equivalent?

David

Re: [PATCH] NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

[Trond Myklebust]

On Thu, 2006-08-24 at 20:45 +0100, David Howells wrote:
> Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> 
> > No. I find that mixture of < and <= is much less easy to read. Besides,
> > the compiler should be able to optimise that for me.
> 
> So you don't think they're mathematically equivalent?

The fact that they are mathematically equivalent does not make them
equivalently easy to read.

As long as the compiler is able to optimise it, we should be quite free
to choose one or the other style of code.

I therefore chose the style which explicitly lists the number of 32-bit
words we want to scan.

Trond