Avoid array overflow in __nfs4_get_acl_uncached

Commit Message

Trond Myklebust Aug. 26, 2012, 6:57 p.m. UTC

On Sun, 2012-08-26 at 00:31 +0100, Sachin Prabhu wrote:
> On Fri, 2012-08-24 at 22:02 +0000, Myklebust, Trond wrote:

> > On Fri, 2012-08-24 at 22:51 +0100, Sachin Prabhu wrote:

> > > On Fri, 2012-08-24 at 21:38 +0000, Myklebust, Trond wrote:

> > > > On Fri, 2012-08-24 at 22:31 +0100, Sachin Prabhu wrote:

> > > > > On Fri, 2012-08-24 at 15:07 +0000, Myklebust, Trond wrote:

> > > > > > On Fri, 2012-08-24 at 15:16 +0100, Sachin Prabhu wrote:

> > > > > > > This fixes a bug introduced by commit

> > > > > > > 5a00689930ab975fdd1b37b034475017e460cf2a

> > > > > > > The patch adds an extra page to npages to hold the bitmap returned by

> > > > > > > the server.

> > > > > > > 

> > > > > > > Bruce Fields pointed out that the changes introduced by the patch will

> > > > > > > cause the array npages to overflow if a buffer of size greater than or

> > > > > > > equal to XATTR_SIZE_MAX is passed to __nfs4_get_acl_uncached()

> > > > > > 

> > > > > > I'd think that the right thing to do here is rather to add appropriate

> > > > > > buffer overflow checks. How about something like the following?

> > > > > > 

> > > > > > 8<--------------------------------------------------------------

> > > > > > From 7c35ce220924182284aea9f8aec39b0d991600df Mon Sep 17 00:00:00 2001

> > > > > > From: Trond Myklebust <[e-mail]>

> > > > > > Date: Fri, 24 Aug 2012 10:59:25 -0400

> > > > > > Subject: [PATCH] NFSv4: Fix range checking in __nfs4_get_acl_uncached and

> > > > > >  __nfs4_proc_set_acl

> > > > > > 

> > > > > > Ensure that the user supplied buffer size doesn't cause us to overflow

> > > > > > the 'pages' array.

> > > > > > 

> > > > > > Also fix up some confusion between the use of PAGE_SIZE and

> > > > > > PAGE_CACHE_SIZE when calculating buffer sizes. We're not using

> > > > > > the page cache for anything here.

> > > > > > 

> > > > > > Signed-off-by: Trond Myklebust <[e-mail]>

> > > > > 

> > > > > This patch is susceptible to the problem described in commit

> > > > > 5a00689930ab975fdd1b37b034475017e460cf2a

> > > > > 

> > > > > This can be demonstrated by the following patch to pynfs which pads the

> > > > > bitmap with 1000 extra elements.

> > > > > 

> > > > > To reproduce, on the server

> > > > > cd newpynfs/nfs4.0/

> > > > > ./setup.py build_ext --inplace

> > > > > ./nfs4server.py

> > > > > 

> > > > > on the client, 

> > > > > mount -o vers=4 SERVER:/ /mnt

> > > > > touch /mnt/a

> > > > > nfs4_getfacl /mnt/a

> > > > > 

> > > > > With this new patch, you will get a general protection fault in

> > > > > _copy_from_pages().

> > > > 

> > > > Is this on a kernel with commit 519d3959e30a98f8e135e7a16647c10af5ad63d5

> > > > (NFSv4: Fix pointer arithmetic in decode_getacl) applied?

> > > > 

> > > 

> > > Yes.

> > 

> > OK, so can you look into which parameters are incorrect and why? The

> > checks in decode_getacl are supposed ensure that we don't overflow the

> > xdr->buf->page_len, so if those are insufficient, then I'd like to

> > understand why.

> > 

> 

> The problem is seen because we do not check to see if the acl_len +

> bitmap size is contained within the page buffer we allocate for it. 

> 

> It will fail if

> bitmap array size + acl length attribute size + ACLs > pages allocated

> for it.  

> 

> In this case, when we call nfs4_write_cached_acl() to write the returned

> ACLs into cache, we call _copy_from_pages() to read from 

> ie. We call nfs4_write_cached_acl

>                 nfs4_write_cached_acl(inode, pages, res.acl_data_offset,

>                                       acl_len);

> which in turn calls

> static void nfs4_write_cached_acl(struct inode *inode, struct page

> **pages, size_t pgbase, size_t acl_len)

> {

> ..

>                 _copy_from_pages(acl->data, pages, pgbase, acl_len);

> ..

> }


Right. That is basically due to incomplete buffer checking in
nfs4_write_cached_acl() itself. We can't catch that case when
NFS4_ACL_LEN_REQUEST is set, because decode_acl ignores the check.

OK. How about this patch, which applies on top of the one I sent you?
8<-----------------------------------------------------------------
From 22c9e2475560edda925f971a2a02bac58536414b Mon Sep 17 00:00:00 2001
From: Trond Myklebust <[e-mail]>

Date: Sun, 26 Aug 2012 11:44:43 -0700
Subject: [PATCH] NFSv4: Add buffer overflow checking to nfs4_write_cached_acl

Currently, the buffer overflow checking is done incorrectly by the
caller. Move the overflow checking into nfs4_write_cached_acl itself
for robustness, and fix the overflow calculation to take into account
the 'pgbase' argument to _copy_from_pages.

Signed-off-by: Trond Myklebust <[e-mail]>

---
 fs/nfs/nfs4proc.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

-- 
1.7.11.4


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
[e-mail]
www.netapp.com

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 654dc38..af4ebc3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3734,12 +3734,13 @@  out:
 	return ret;
 }
 
-static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size_t pgbase, size_t acl_len)
+static void nfs4_write_cached_acl(struct inode *inode, struct page **pages,
+		size_t srclen, size_t pgbase, size_t acl_len)
 {
 	struct nfs4_cached_acl *acl;
 	size_t buflen = sizeof(*acl) + acl_len;
 
-	if (pages && buflen <= PAGE_SIZE) {
+	if (buflen <= PAGE_SIZE && srclen <= pgbase + acl_len) {
 		acl = kmalloc(buflen, GFP_KERNEL);
 		if (acl == NULL)
 			goto out;
@@ -3820,11 +3821,8 @@  static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
 		goto out_free;
 
 	acl_len = res.acl_len;
-	if (acl_len > args.acl_len)
-		nfs4_write_cached_acl(inode, NULL, 0, acl_len);
-	else
-		nfs4_write_cached_acl(inode, pages, res.acl_data_offset,
-				      acl_len);
+	nfs4_write_cached_acl(inode, pages, args.acl_len,
+			res.acl_data_offset, res.acl_len);
 	if (buf) {
 		ret = -ERANGE;
 		if (acl_len > buflen)