Skip to contentSkip to conversation

NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

PATCH In review David Howells 2 participants 6 messages Aug 22, 2006 – Aug 24, 2006

Subsystems (3)
Networking NFS client NFS server
lore
Share & tools
lore

David Howells

author
#0 lore raw

PATCH 1/1 NFS: Check lengths more thoroughly in NFS4 readdir XDR decode

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 <[email protected]>

---

 fs/nfs/nfs4xdr.c  |   22 +++++++++++-----------
 net/sunrpc/clnt.c |   11 +++++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)
fs/nfs/nfs4xdr.c +11 -11
index 992a713..41a295a 100644
@@ -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))
net/sunrpc/clnt.c +11 -0
index d9eac70..3e19d32 100644
@@ -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 */
#1 lore raw
Quoted 2 lines · David Howells

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

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 <[email protected]>
Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/nfs4xdr.c  |   21 +++++++++++----------
 net/sunrpc/clnt.c |   11 +++++++++++
 2 files changed, 22 insertions(+), 10 deletions(-)
fs/nfs/nfs4xdr.c +11 -10
index 83c90d0..3dd413f 100644
@@ -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))
net/sunrpc/clnt.c +11 -0
index 88c98ac..87efcd2 100644
@@ -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 */
author
#2 lore raw

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

#3 lore raw
Quoted 31 lines · 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)

Quoted 8 lines

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

                              ^^^^^^^^^^^^^^
Quoted 2 lines

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

author
#4 lore raw
Quoted 2 lines · Trond Myklebust

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

#5 lore raw
Quoted 6 lines · David Howells
Quoted 2 lines · Trond Myklebust

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