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@xxxxxxxxxx>
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@xxxxxxxxxx>
---
fs/nfs/nfs4proc.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 6352741..654dc38 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3653,11 +3653,11 @@ static inline int nfs4_server_supports_acls(struct nfs_server *server)
&& (server->acl_bitmask & ACL4_SUPPORT_DENY_ACL);
}
-/* Assuming that XATTR_SIZE_MAX is a multiple of PAGE_CACHE_SIZE, and that
- * it's OK to put sizeof(void) * (XATTR_SIZE_MAX/PAGE_CACHE_SIZE) bytes on
+/* Assuming that XATTR_SIZE_MAX is a multiple of PAGE_SIZE, and that
+ * it's OK to put sizeof(void) * (XATTR_SIZE_MAX/PAGE_SIZE) bytes on
* the stack.
*/
-#define NFS4ACL_MAXPAGES (XATTR_SIZE_MAX >> PAGE_CACHE_SHIFT)
+#define NFS4ACL_MAXPAGES DIV_ROUND_UP(XATTR_SIZE_MAX, PAGE_SIZE)
static int buf_to_pages_noslab(const void *buf, size_t buflen,
struct page **pages, unsigned int *pgbase)
@@ -3668,7 +3668,7 @@ static int buf_to_pages_noslab(const void *buf, size_t buflen,
spages = pages;
do {
- len = min_t(size_t, PAGE_CACHE_SIZE, buflen);
+ len = min_t(size_t, PAGE_SIZE, buflen);
newpage = alloc_page(GFP_KERNEL);
if (newpage == NULL)
@@ -3782,17 +3782,16 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
.rpc_argp = &args,
.rpc_resp = &res,
};
- int ret = -ENOMEM, npages, i;
+ unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
+ int ret = -ENOMEM, i;
size_t acl_len = 0;
- npages = (buflen + PAGE_SIZE - 1) >> PAGE_SHIFT;
/* As long as we're doing a round trip to the server anyway,
* let's be prepared for a page of acl data. */
if (npages == 0)
npages = 1;
-
- /* Add an extra page to handle the bitmap returned */
- npages++;
+ if (npages > ARRAY_SIZE(pages))
+ return -ERANGE;
for (i = 0; i < npages; i++) {
pages[i] = alloc_page(GFP_KERNEL);
@@ -3891,10 +3890,13 @@ static int __nfs4_proc_set_acl(struct inode *inode, const void *buf, size_t bufl
.rpc_argp = &arg,
.rpc_resp = &res,
};
+ unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
int ret, i;
if (!nfs4_server_supports_acls(server))
return -EOPNOTSUPP;
+ if (npages > ARRAY_SIZE(pages))
+ return -ERANGE;
i = buf_to_pages_noslab(buf, buflen, arg.acl_pages, &arg.acl_pgbase);
if (i < 0)
return i;
--
1.7.11.4
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
