On Mon, 2012-09-03 at 19:11 +0000, Myklebust, Trond wrote: > > 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? I could reproduce the crash with the second _copy_from_pages using the patch to PyNFS from http://www.spinics.net/lists/linux-nfs/msg32359.html The patch below works fine for both cases apart from a small bug which I've pointed to below. > 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) { Here we need to use res->acl_data_offset which points to the start of the ACL data instead of pg_offset which points to the start of the pages section of the xdr_buf. Once I changed this if condition, I was able to successfully test with my reproducer. Sachin Prabhu > + 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
