| Message ID | 4FA345DA4F4AE44899BD2B03EEEC2FA908F81C4A@SACEXCMBX04-PRD.hq.netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@netapp.com> > 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@netapp.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-09-06 at 15:46 +0100, Sachin Prabhu wrote: > 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@netapp.com> > > Date: Sun, 26 Aug 2012 11:44:43 -0700 > > Subject: [PATCH] NFSv4: Fix buffer overflow checking in > > __nfs4_get_acl_uncached > > + /* 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. Oops... Yes, of course you are right. I'll make the modification and push out the changed patch. Thanks very much for testing! Do you want me to add a "Tested-by:" line? -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com
On Thu, 2012-09-06 at 14:53 +0000, Myklebust, Trond wrote: > On Thu, 2012-09-06 at 15:46 +0100, Sachin Prabhu wrote: > > 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@netapp.com> > > > Date: Sun, 26 Aug 2012 11:44:43 -0700 > > > Subject: [PATCH] NFSv4: Fix buffer overflow checking in > > > __nfs4_get_acl_uncached > > > > + /* 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. > > Oops... Yes, of course you are right. I'll make the modification and > push out the changed patch. > > Thanks very much for testing! Do you want me to add a "Tested-by:" line? > Fine by me. 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/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;