From patchwork Wed Sep 25 17:12:43 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Timothy Pepper X-Patchwork-Id: 277939 Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from ozlabs.org (localhost [IPv6:::1]) by ozlabs.org (Postfix) with ESMTP id A6A192C033A for ; Thu, 26 Sep 2013 03:14:07 +1000 (EST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by ozlabs.org (Postfix) with ESMTP id DD9DC2C00BD for ; Thu, 26 Sep 2013 03:12:58 +1000 (EST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 25 Sep 2013 10:12:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.90,979,1371106800"; d="scan'208";a="383450532" Received: from tcpepper-desk.jf.intel.com ([10.7.197.221]) by orsmga001.jf.intel.com with SMTP; 25 Sep 2013 10:12:43 -0700 Received: by tcpepper-desk.jf.intel.com (sSMTP sendmail emulation); Wed, 25 Sep 2013 10:12:43 -0700 From: "Timothy Pepper" Date: Wed, 25 Sep 2013 10:12:43 -0700 To: Ingo Molnar Subject: Re: mm: insure topdown mmap chooses addresses above security minimum Message-ID: <20130925171243.GA7428@tcpepper-desk.jf.intel.com> References: <1380057811-5352-1-git-send-email-timothy.c.pepper@linux.intel.com> <20130925073048.GB27960@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20130925073048.GB27960@gmail.com> X-PGP-Key: http://vato.org/pubkey.asc User-Agent: Mutt/1.5.21 (2010-09-15) Cc: linux-mips@linux-mips.org, linux-sh@vger.kernel.org, linux-mm@kvack.org, Paul Mackerras , "H. Peter Anvin" , sparclinux@vger.kernel.org, Michel Lespinasse , Russell King , x86@kernel.org, Ingo Molnar , Rik van Riel , Al Viro , James Morris , Thomas Gleixner , linux-arm-kernel@lists.infradead.org, linuxppc-dev@lists.ozlabs.org, Ralf Baechle , Paul Mundt , Andrew Morton , Linus Torvalds , "David S. Miller" X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.16rc2 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Wed 25 Sep at 09:30:49 +0200 mingo@kernel.org said: > > info.flags = VM_UNMAPPED_AREA_TOPDOWN; > > info.length = len; > > - info.low_limit = PAGE_SIZE; > > + info.low_limit = max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr)); > > info.high_limit = mm->mmap_base; > > info.align_mask = filp ? get_align_mask() : 0; > > info.align_offset = pgoff << PAGE_SHIFT; > > There appears to be a lot of repetition in these methods - instead of > changing 6 places it would be more future-proof to first factor out the > common bits and then to apply the fix to the shared implementation. Besides that existing redundancy in the multiple somewhat similar arch_get_unmapped_area_topdown() functions, I was expecting people might question the added redundancy of the six instances of: max(PAGE_SIZE, PAGE_ALIGN(mmap_min_addr)); There's also a seventh similar instance if you consider mm/mmap.c:round_hint_to_min() and its call stack. I'm inclined to think mmap_min_addr should be validated/aligned in one place, namely on initialization and input in security/min_addr.c:update_mmap_min_addr(), with mmap_min_addr always stored as an aligned value. In the past commit 40401530 Al Viro arguably moved that checking out of the security code and toward the mmap code. Granted at that point though there was only the round_hint_to_min() insuring the value in mmap_min_addr was page aligned before use in that call path. I'm thinking something like: But this possibly has implications beyond the mmap code. Al Viro, James Morris: any thoughts on the above? Michel, Rik: what do you think of common helpers called by ARM, MIPS, SH, Sparc, x86_64 arch_get_unmapped_area_topdown() and arch_get_unmapped_area() to handle initialization of struct vm_unmapped_area_info info fields which are currently mostly common? Given the nuances of "mostly common" I'm not sure the result would actually be positive for overall readability / self-documenting-ness of the per arch files. diff --git a/security/min_addr.c b/security/min_addr.c --- a/security/min_addr.c +++ b/security/min_addr.c @@ -14,14 +14,16 @@ unsigned long dac_mmap_min_addr = CONFIG_DEFAULT_MMAP_MIN_ADDR; */ static void update_mmap_min_addr(void) { + unsigned long addr; #ifdef CONFIG_LSM_MMAP_MIN_ADDR if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR) - mmap_min_addr = dac_mmap_min_addr; + addr = dac_mmap_min_addr; else - mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR; + addr = CONFIG_LSM_MMAP_MIN_ADDR; #else - mmap_min_addr = dac_mmap_min_addr; + addr = dac_mmap_min_addr; #endif + mmap_min_addr = max(PAGE_SIZE, PAGE_ALIGN(addr)); } /*