From patchwork Mon Sep 3 19:11:52 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 1400711 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id D603D3FC85 for ; Mon, 3 Sep 2012 19:11:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756356Ab2ICTLz (ORCPT ); Mon, 3 Sep 2012 15:11:55 -0400 Received: from mx2.netapp.com ([216.240.18.37]:31946 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755001Ab2ICTLy (ORCPT ); Mon, 3 Sep 2012 15:11:54 -0400 X-IronPort-AV: E=Sophos;i="4.80,361,1344236400"; d="scan'208";a="685275208" Received: from smtp1.corp.netapp.com ([10.57.156.124]) by mx2-out.netapp.com with ESMTP; 03 Sep 2012 12:11:54 -0700 Received: from vmwexceht02-prd.hq.netapp.com (vmwexceht02-prd.hq.netapp.com [10.106.76.240]) by smtp1.corp.netapp.com (8.13.1/8.13.1/NTAP-1.6) with ESMTP id q83JBsYD020154; Mon, 3 Sep 2012 12:11:54 -0700 (PDT) Received: from SACEXCMBX04-PRD.hq.netapp.com ([169.254.6.158]) by vmwexceht02-prd.hq.netapp.com ([10.106.76.240]) with mapi id 14.02.0309.002; Mon, 3 Sep 2012 12:11:53 -0700 From: "Myklebust, Trond" To: Sachin Prabhu CC: Linux NFS mailing list Subject: Re: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached Thread-Topic: [PATCH] Avoid array overflow in __nfs4_get_acl_uncached Thread-Index: AQHNggMEevzXyQIPzEqs3bi1RBP5MJdphd4AgABq/gCAAAJMgIAAA0YAgAADMQCAAatFgIABRaQAgALUQQCACcJ7gA== Date: Mon, 3 Sep 2012 19:11:52 +0000 Message-ID: <4FA345DA4F4AE44899BD2B03EEEC2FA908F81C4A@SACEXCMBX04-PRD.hq.netapp.com> References: <1345817768-23511-1-git-send-email-sprabhu@redhat.com> <4FA345DA4F4AE44899BD2B03EEEC2FA908F4AF1E@SACEXCMBX01-PRD.hq.netapp.com> <1345843866.2279.6.camel@localhost> <4FA345DA4F4AE44899BD2B03EEEC2FA908F5842F@SACEXCMBX01-PRD.hq.netapp.com> <1345845062.32200.1.camel@localhost> <4FA345DA4F4AE44899BD2B03EEEC2FA908F5859F@SACEXCMBX01-PRD.hq.netapp.com> <1345937503.4943.17.camel@localhost> <4FA345DA4F4AE44899BD2B03EEEC2FA908F69340@SACEXCMBX04-PRD.hq.netapp.com> <1346162966.2554.3.camel@localhost> In-Reply-To: <1346162966.2554.3.camel@localhost> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.104.60.115] Content-ID: MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org 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 > > 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 > > --- > > 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 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 --- fs/nfs/nfs4proc.c | 31 ++++++++++++------------------- fs/nfs/nfs4xdr.c | 12 ++++-------- include/linux/nfs_xdr.h | 2 +- 3 files changed, 17 insertions(+), 28 deletions(-) -- 1.7.11.4 -- Trond Myklebust Linux NFS client maintainer NetApp Trond.Myklebust@netapp.com www.netapp.com 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;