On Tue, 2012-08-28 at 15:09 +0100, Sachin Prabhu wrote:
> > 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 <Trond.Myklebust@xxxxxxxxxx>
> > 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 <Trond.Myklebust@xxxxxxxxxx>
> > ---
> > fs/nfs/nfs4proc.c | 12 +++++-------
> > 1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > 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)
> > --
> > 1.7.11.4
>
> I encountered 2 problems.
> 1) The if condition should be srclen >= pgbase + acl_len
> 2) There is a second _copy_from_pages which copies to the the acl to the
> passed buffer in __nfs4_get_acl_uncached().
The second copy from pages should already be covered by the checks in
decode_getacl. Alright, since this is not obvious, then clearly we need
to make it so. How about the following?
8<------------------------------------------------------------------
>From 5040240245a046bd58c383806b3f161ee8b5823b Mon Sep 17 00:00:00 2001
From: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
Date: Sun, 26 Aug 2012 11:44:43 -0700
Subject: [PATCH] NFSv4: Fix buffer overflow checking in
__nfs4_get_acl_uncached
Pass the checks made by decode_getacl back to __nfs4_get_acl_uncached
so that it knows if the acl has been truncated.
Signed-off-by: Trond Myklebust <Trond.Myklebust@xxxxxxxxxx>
---
fs/nfs/nfs4proc.c | 31 ++++++++++++-------------------
fs/nfs/nfs4xdr.c | 12 ++++--------
include/linux/nfs_xdr.h | 2 +-
3 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 654dc38..74f5c26 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3739,7 +3739,7 @@ static void nfs4_write_cached_acl(struct inode *inode, struct page **pages, size
struct nfs4_cached_acl *acl;
size_t buflen = sizeof(*acl) + acl_len;
- if (pages && buflen <= PAGE_SIZE) {
+ if (buflen <= PAGE_SIZE) {
acl = kmalloc(buflen, GFP_KERNEL);
if (acl == NULL)
goto out;
@@ -3784,7 +3784,6 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
};
unsigned int npages = DIV_ROUND_UP(buflen, PAGE_SIZE);
int ret = -ENOMEM, i;
- size_t acl_len = 0;
/* As long as we're doing a round trip to the server anyway,
* let's be prepared for a page of acl data. */
@@ -3807,11 +3806,6 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
args.acl_len = npages * PAGE_SIZE;
args.acl_pgbase = 0;
- /* Let decode_getfacl know not to fail if the ACL data is larger than
- * the page we send as a guess */
- if (buf == NULL)
- res.acl_flags |= NFS4_ACL_LEN_REQUEST;
-
dprintk("%s buf %p buflen %zu npages %d args.acl_len %zu\n",
__func__, buf, buflen, npages, args.acl_len);
ret = nfs4_call_sync(NFS_SERVER(inode)->client, NFS_SERVER(inode),
@@ -3819,20 +3813,19 @@ static ssize_t __nfs4_get_acl_uncached(struct inode *inode, void *buf, size_t bu
if (ret)
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);
- if (buf) {
+ /* Handle the case where the passed-in buffer is too short */
+ if (res.acl_flags & NFS4_ACL_TRUNC) {
+ /* Did the user only issue a request for the acl length? */
+ if (buf == NULL)
+ goto out_ok;
ret = -ERANGE;
- if (acl_len > buflen)
- goto out_free;
- _copy_from_pages(buf, pages, res.acl_data_offset,
- acl_len);
+ goto out_free;
}
- ret = acl_len;
+ nfs4_write_cached_acl(inode, pages, res.acl_data_offset, res.acl_len);
+ if (buf)
+ _copy_from_pages(buf, pages, res.acl_data_offset, res.acl_len);
+out_ok:
+ ret = res.acl_len;
out_free:
for (i = 0; i < npages; i++)
if (pages[i])
diff --git a/fs/nfs/nfs4xdr.c b/fs/nfs/nfs4xdr.c
index 1bfbd67..3ebe025 100644
--- a/fs/nfs/nfs4xdr.c
+++ b/fs/nfs/nfs4xdr.c
@@ -5073,17 +5073,13 @@ static int decode_getacl(struct xdr_stream *xdr, struct rpc_rqst *req,
* variable length bitmaps.*/
res->acl_data_offset = xdr_stream_pos(xdr) - pg_offset;
- /* We ignore &savep and don't do consistency checks on
- * the attr length. Let userspace figure it out.... */
res->acl_len = attrlen;
- if (attrlen > (xdr->nwords << 2)) {
- if (res->acl_flags & NFS4_ACL_LEN_REQUEST) {
- /* getxattr interface called with a NULL buf */
- goto out;
- }
+ /* Check for receive buffer overflow */
+ if (attrlen > (xdr->nwords << 2) ||
+ attrlen + pg_offset > xdr->buf->page_len) {
+ res->acl_flags |= NFS4_ACL_TRUNC;
dprintk("NFS: acl reply: attrlen %u > page_len %u\n",
attrlen, xdr->nwords << 2);
- return -EINVAL;
}
} else
status = -EOPNOTSUPP;
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index ac7c8ae..be9cf3c 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -652,7 +652,7 @@ struct nfs_getaclargs {
};
/* getxattr ACL interface flags */
-#define NFS4_ACL_LEN_REQUEST 0x0001 /* zero length getxattr buffer */
+#define NFS4_ACL_TRUNC 0x0001 /* ACL was truncated */
struct nfs_getaclres {
size_t acl_len;
size_t acl_data_offset;
--
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�����٥
