From patchwork Sun Aug 26 18:57:16 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 1375751 Return-Path: X-Original-To: patchwork-linux-nfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 92BE3DFABE for ; Sun, 26 Aug 2012 18:57:35 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752344Ab2HZS5e (ORCPT ); Sun, 26 Aug 2012 14:57:34 -0400 Received: from mx2.netapp.com ([216.240.18.37]:9763 "EHLO mx2.netapp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752155Ab2HZS5d (ORCPT ); Sun, 26 Aug 2012 14:57:33 -0400 X-IronPort-AV: E=Sophos;i="4.80,316,1344236400"; d="scan'208";a="681523525" Received: from smtp1.corp.netapp.com ([10.57.156.124]) by mx2-out.netapp.com with ESMTP; 26 Aug 2012 11:57:18 -0700 Received: from vmwexceht05-prd.hq.netapp.com (vmwexceht05-prd.hq.netapp.com [10.106.77.35]) by smtp1.corp.netapp.com (8.13.1/8.13.1/NTAP-1.6) with ESMTP id q7QIvHjo028992; Sun, 26 Aug 2012 11:57:18 -0700 (PDT) Received: from SACEXCMBX04-PRD.hq.netapp.com ([169.254.6.158]) by vmwexceht05-prd.hq.netapp.com ([10.106.77.35]) with mapi id 14.02.0309.002; Sun, 26 Aug 2012 11:56:54 -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/gCAAAJMgIAAA0YAgAADMQCAAatFgIABRaQA Date: Sun, 26 Aug 2012 18:57:16 +0000 Message-ID: <4FA345DA4F4AE44899BD2B03EEEC2FA908F69340@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> In-Reply-To: <1345937503.4943.17.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 Sun, 2012-08-26 at 00:31 +0100, Sachin Prabhu wrote: > On Fri, 2012-08-24 at 22:02 +0000, Myklebust, Trond wrote: > > On Fri, 2012-08-24 at 22:51 +0100, Sachin Prabhu wrote: > > > On Fri, 2012-08-24 at 21:38 +0000, Myklebust, Trond wrote: > > > > On Fri, 2012-08-24 at 22:31 +0100, Sachin Prabhu wrote: > > > > > On Fri, 2012-08-24 at 15:07 +0000, Myklebust, Trond wrote: > > > > > > 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 > > > > > > 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 > > > > > > > > > > This patch is susceptible to the problem described in commit > > > > > 5a00689930ab975fdd1b37b034475017e460cf2a > > > > > > > > > > This can be demonstrated by the following patch to pynfs which pads the > > > > > bitmap with 1000 extra elements. > > > > > > > > > > To reproduce, on the server > > > > > cd newpynfs/nfs4.0/ > > > > > ./setup.py build_ext --inplace > > > > > ./nfs4server.py > > > > > > > > > > on the client, > > > > > mount -o vers=4 SERVER:/ /mnt > > > > > touch /mnt/a > > > > > nfs4_getfacl /mnt/a > > > > > > > > > > With this new patch, you will get a general protection fault in > > > > > _copy_from_pages(). > > > > > > > > Is this on a kernel with commit 519d3959e30a98f8e135e7a16647c10af5ad63d5 > > > > (NFSv4: Fix pointer arithmetic in decode_getacl) applied? > > > > > > > > > > Yes. > > > > OK, so can you look into which parameters are incorrect and why? The > > checks in decode_getacl are supposed ensure that we don't overflow the > > xdr->buf->page_len, so if those are insufficient, then I'd like to > > understand why. > > > > The problem is seen because we do not check to see if the acl_len + > bitmap size is contained within the page buffer we allocate for it. > > It will fail if > bitmap array size + acl length attribute size + ACLs > pages allocated > for it. > > In this case, when we call nfs4_write_cached_acl() to write the returned > ACLs into cache, we call _copy_from_pages() to read from > ie. We call nfs4_write_cached_acl > nfs4_write_cached_acl(inode, pages, res.acl_data_offset, > acl_len); > which in turn calls > static void nfs4_write_cached_acl(struct inode *inode, struct page > **pages, size_t pgbase, size_t acl_len) > { > .. > _copy_from_pages(acl->data, pages, pgbase, acl_len); > .. > } Right. That is basically due to incomplete buffer checking in nfs4_write_cached_acl() itself. We can't catch that case when NFS4_ACL_LEN_REQUEST is set, because decode_acl ignores the check. 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(-) -- 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..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)