From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Timothy Pepper" Date: Wed, 25 Sep 2013 17:12:43 +0000 Subject: Re: mm: insure topdown mmap chooses addresses above security minimum Message-Id: <20130925171243.GA7428@tcpepper-desk.jf.intel.com> List-Id: References: <1380057811-5352-1-git-send-email-timothy.c.pepper@linux.intel.com> <20130925073048.GB27960@gmail.com> In-Reply-To: <20130925073048.GB27960@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org 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: 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)); } /* 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. -- Tim Pepper Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 Received: with ECARTIS (v1.0.0; list linux-mips); Wed, 25 Sep 2013 19:15:55 +0200 (CEST) Received: from mga09.intel.com ([134.134.136.24]:23788 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by eddie.linux-mips.org with ESMTP id S6816233Ab3IYRN3VoCnO (ORCPT ); Wed, 25 Sep 2013 19:13:29 +0200 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 25 Sep 2013 10:09:38 -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 Cc: linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Russell King , linux-arm-kernel@lists.infradead.org, Ralf Baechle , linux-mips@linux-mips.org, Benjamin Herrenschmidt , Paul Mackerras , linuxppc-dev@lists.ozlabs.org, Paul Mundt , linux-sh@vger.kernel.org, "David S. Miller" , sparclinux@vger.kernel.org, Andrew Morton , Linus Torvalds , Al Viro , James Morris , Michel Lespinasse , Rik van Riel 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-Type: text/plain; charset=us-ascii 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) Return-Path: X-Envelope-To: <"|/home/ecartis/ecartis -s linux-mips"> (uid 0) X-Orcpt: rfc822;linux-mips@linux-mips.org Original-Recipient: rfc822;linux-mips@linux-mips.org X-archive-position: 37974 X-ecartis-version: Ecartis v1.0.0 Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org X-original-sender: timothy.c.pepper@linux.intel.com Precedence: bulk List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: linux-mips X-List-ID: linux-mips List-subscribe: List-owner: List-post: List-archive: X-list: linux-mips 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: 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)); } /* 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. -- Tim Pepper Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f180.google.com (mail-pd0-f180.google.com [209.85.192.180]) by kanga.kvack.org (Postfix) with ESMTP id B56BD6B0034 for ; Wed, 25 Sep 2013 13:12:50 -0400 (EDT) Received: by mail-pd0-f180.google.com with SMTP id y10so6305586pdj.25 for ; Wed, 25 Sep 2013 10:12:50 -0700 (PDT) From: "Timothy Pepper" Date: Wed, 25 Sep 2013 10:12:43 -0700 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-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130925073048.GB27960@gmail.com> Sender: owner-linux-mm@kvack.org List-ID: To: Ingo Molnar Cc: linux-mm@kvack.org, Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Russell King , linux-arm-kernel@lists.infradead.org, Ralf Baechle , linux-mips@linux-mips.org, Benjamin Herrenschmidt , Paul Mackerras , linuxppc-dev@lists.ozlabs.org, Paul Mundt , linux-sh@vger.kernel.org, "David S. Miller" , sparclinux@vger.kernel.org, Andrew Morton , Linus Torvalds , Al Viro , James Morris , Michel Lespinasse , Rik van Riel 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: 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)); } /* 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. -- Tim Pepper Intel Open Source Technology Center -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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) 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-Type: text/plain; charset=us-ascii In-Reply-To: <20130925073048.GB27960@gmail.com> 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" List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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: 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)); } /* 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. -- Tim Pepper Intel Open Source Technology Center From mboxrd@z Thu Jan 1 00:00:00 1970 From: timothy.c.pepper@linux.intel.com (Timothy Pepper) Date: Wed, 25 Sep 2013 10:12:43 -0700 Subject: mm: insure topdown mmap chooses addresses above security minimum In-Reply-To: <20130925073048.GB27960@gmail.com> References: <1380057811-5352-1-git-send-email-timothy.c.pepper@linux.intel.com> <20130925073048.GB27960@gmail.com> Message-ID: <20130925171243.GA7428@tcpepper-desk.jf.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed 25 Sep at 09:30:49 +0200 mingo at 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: 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)); } /* 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. -- Tim Pepper Intel Open Source Technology Center