| Message ID | 1345843866.2279.6.camel@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <Trond.Myklebust@netapp.com> > > 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 <Trond.Myklebust@netapp.com> > > 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? -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
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 <Trond.Myklebust@netapp.com> > > > 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 <Trond.Myklebust@netapp.com> > > > > 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. Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <Trond.Myklebust@netapp.com> > > > > 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 <Trond.Myklebust@netapp.com> > > > > > > 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. -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
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 <Trond.Myklebust@netapp.com> > > > > > 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 <Trond.Myklebust@netapp.com> > > > > > > > > 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); .. } A simple example is if the ACL size is just less than a PAGE_SIZE but large enough so that ACL + Bitmap crosses a PAGE_SIZE, it will fail. Another example is if the server sends a large bitmap array along with smaller ACL data causing the bitmap array + ACL to go past the allocated page, it will fail again. We exploit the bug using the second example given above. We configured pynfs to append 1000 extra elements to the bitmap array which results in BITMAP+ACL size to be little over a PAGE_SIZE. This leads to a General Protection Fault in _copy_from_pages(). Using a simple printk in __nfs4_get_acl_uncached, I see the following values args.acl_len=4096, res.acl_data_offset=4012, res.acl_len=156 We end up calling _copy_from_pages(acl->data, pages, 4012, 156); When tested _without_ this patch, where the minimum npages sent is 2, and the bitmap padding is set so that we cross 2*PAGESIZE, we fail in decode_attr_bitmap-> xdr_decode_inline because we cross buffer boundary and the allocated scratch page is insufficient to read the bitmap returned. Although it prevent the crash, it is not the ideal solution to the problem. Sachin Prabhu -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/nfs4.0/lib/nfs4/nfs4lib.py b/nfs4.0/lib/nfs4/nfs4lib.py index 0a38ebc..940ae86 100644 --- a/nfs4.0/lib/nfs4/nfs4lib.py +++ b/nfs4.0/lib/nfs4/nfs4lib.py @@ -87,9 +87,17 @@ class FancyNFS4Packer(nfs4_pack.NFS4Packer): """Handle fattr4 and dirlist4 more cleanly than auto-generated methods""" def filter_bitmap4(self, data): out = [] + flag = 0; while data: + flag = 1; out.append(data & 0xffffffffL) data >>= 32 + if (flag and out[0] == 4096) : + print "add test data"; + i = 1000 + while i: + out.append(0) + i -= 1 return out def filter_fattr4(self, data):