From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751637AbcGaU53 (ORCPT ); Sun, 31 Jul 2016 16:57:29 -0400 Received: from outbound1a.ore.mailhop.org ([54.213.22.21]:48020 "EHLO outbound1a.ore.mailhop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751168AbcGaU5Y (ORCPT ); Sun, 31 Jul 2016 16:57:24 -0400 X-MHO-User: 6a27332c-5761-11e6-a0ff-e511cd071b9b X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 74.99.77.15 X-Mail-Handler: DuoCircle Outbound SMTP X-DKIM: OpenDKIM Filter v2.6.8 io 70A468006D Date: Sun, 31 Jul 2016 20:56:32 +0000 From: Jason Cooper To: Kees Cook Cc: "Roberts, William C" , Yann Droneaud , Linux-MM , LKML , "kernel-hardening@lists.openwall.com" , Russell King - ARM Linux , Andrew Morton , "Theodore Ts'o" , Arnd Bergmann , Greg KH , Catalin Marinas , Will Deacon , Ralf Baechle , "benh@kernel.crashing.org" , Paul Mackerras , Michael Ellerman , "David S. Miller" , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , Al Viro , Nick Kralevich , Jeffrey Vander Stoep , Daniel Cashman Subject: Re: [PATCH v2 1/7] random: Simplify API for random address requests Message-ID: <20160731205632.GY4541@io.lakedaemon.net> References: <20160728204730.27453-1-jason@lakedaemon.net> <20160730154244.403-1-jason@lakedaemon.net> <20160730154244.403-2-jason@lakedaemon.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jul 31, 2016 at 09:46:53AM -0700, Kees Cook wrote: > On Sat, Jul 30, 2016 at 8:42 AM, Jason Cooper wrote: > > To date, all callers of randomize_range() have set the length to 0, and > > check for a zero return value. For the current callers, the only way > > to get zero returned is if end <= start. Since they are all adding a > > constant to the start address, this is unnecessary. > > > > We can remove a bunch of needless checks by simplifying the API to do > > just what everyone wants, return an address between [start, start + > > range). > > > > While we're here, s/get_random_int/get_random_long/. No current call > > site is adversely affected by get_random_int(), since all current range > > requests are < UINT_MAX. However, we should match caller expectations > > to avoid coming up short (ha!) in the future. > > > > All current callers to randomize_range() chose to use the start address > > if randomize_range() failed. Therefore, we simplify things by just > > returning the start address on error. > > > > randomize_range() will be removed once all callers have been converted > > over to randomize_addr(). > > > > Signed-off-by: Jason Cooper > > --- > > Changes from v1: > > - Explicitly mention page_aligned start assumption (Yann Droneaud) > > - pick random pages vice random addresses (Yann Droneaud) > > - catch range=0 last > > > > drivers/char/random.c | 28 ++++++++++++++++++++++++++++ > > include/linux/random.h | 1 + > > 2 files changed, 29 insertions(+) > > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > index 0158d3bff7e5..3bedf69546d6 100644 > > --- a/drivers/char/random.c > > +++ b/drivers/char/random.c > > @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) > > return PAGE_ALIGN(get_random_int() % range + start); > > } > > > > +/** > > + * randomize_addr - Generate a random, page aligned address > > + * @start: The smallest acceptable address the caller will take. > > + * @range: The size of the area, starting at @start, within which the > > + * random address must fall. > > + * > > + * If @start + @range would overflow, @range is capped. > > + * > > + * NOTE: Historical use of randomize_range, which this replaces, presumed that > > + * @start was already page aligned. This assumption still holds. > > + * > > + * Return: A page aligned address within [start, start + range). On error, > > + * @start is returned. > > + */ > > +unsigned long > > +randomize_addr(unsigned long start, unsigned long range) > > Since we're changing other things about this, let's try to document > its behavior in its name too and call this "randomize_page" instead. Ack. Definitely more accurate. > If it requires a page-aligned value, we should probably also BUG_ON > it, or adjust the start too. merf. So, this whole series started from a suggested cleanup by William to s/get_random_int/get_random_long/. The current users have all been stable the way they are for a long time. Like pre-git long. So, if this is just a cleanup for those callers, I don't think we need to do more than we already are. However, if the intent is for this function to see wider use, then by all means, we need to handle start != PAGE_ALIGN(start). Do you have any new call sites in mind? thx, Jason. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f199.google.com (mail-pf0-f199.google.com [209.85.192.199]) by kanga.kvack.org (Postfix) with ESMTP id ADCFC6B0260 for ; Sun, 31 Jul 2016 16:56:38 -0400 (EDT) Received: by mail-pf0-f199.google.com with SMTP id w128so233340424pfd.3 for ; Sun, 31 Jul 2016 13:56:38 -0700 (PDT) Received: from outbound1a.ore.mailhop.org (outbound1a.ore.mailhop.org. [54.213.22.21]) by mx.google.com with ESMTPS id j71si31259543pfa.161.2016.07.31.13.56.37 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 31 Jul 2016 13:56:37 -0700 (PDT) Date: Sun, 31 Jul 2016 20:56:32 +0000 From: Jason Cooper Subject: Re: [PATCH v2 1/7] random: Simplify API for random address requests Message-ID: <20160731205632.GY4541@io.lakedaemon.net> References: <20160728204730.27453-1-jason@lakedaemon.net> <20160730154244.403-1-jason@lakedaemon.net> <20160730154244.403-2-jason@lakedaemon.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: owner-linux-mm@kvack.org List-ID: To: Kees Cook Cc: "Roberts, William C" , Yann Droneaud , Linux-MM , LKML , "kernel-hardening@lists.openwall.com" , Russell King - ARM Linux , Andrew Morton , Theodore Ts'o , Arnd Bergmann , Greg KH , Catalin Marinas , Will Deacon , Ralf Baechle , "benh@kernel.crashing.org" , Paul Mackerras , Michael Ellerman , "David S. Miller" , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , Al Viro , Nick Kralevich , Jeffrey Vander Stoep , Daniel Cashman On Sun, Jul 31, 2016 at 09:46:53AM -0700, Kees Cook wrote: > On Sat, Jul 30, 2016 at 8:42 AM, Jason Cooper wrote: > > To date, all callers of randomize_range() have set the length to 0, and > > check for a zero return value. For the current callers, the only way > > to get zero returned is if end <= start. Since they are all adding a > > constant to the start address, this is unnecessary. > > > > We can remove a bunch of needless checks by simplifying the API to do > > just what everyone wants, return an address between [start, start + > > range). > > > > While we're here, s/get_random_int/get_random_long/. No current call > > site is adversely affected by get_random_int(), since all current range > > requests are < UINT_MAX. However, we should match caller expectations > > to avoid coming up short (ha!) in the future. > > > > All current callers to randomize_range() chose to use the start address > > if randomize_range() failed. Therefore, we simplify things by just > > returning the start address on error. > > > > randomize_range() will be removed once all callers have been converted > > over to randomize_addr(). > > > > Signed-off-by: Jason Cooper > > --- > > Changes from v1: > > - Explicitly mention page_aligned start assumption (Yann Droneaud) > > - pick random pages vice random addresses (Yann Droneaud) > > - catch range=0 last > > > > drivers/char/random.c | 28 ++++++++++++++++++++++++++++ > > include/linux/random.h | 1 + > > 2 files changed, 29 insertions(+) > > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > index 0158d3bff7e5..3bedf69546d6 100644 > > --- a/drivers/char/random.c > > +++ b/drivers/char/random.c > > @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) > > return PAGE_ALIGN(get_random_int() % range + start); > > } > > > > +/** > > + * randomize_addr - Generate a random, page aligned address > > + * @start: The smallest acceptable address the caller will take. > > + * @range: The size of the area, starting at @start, within which the > > + * random address must fall. > > + * > > + * If @start + @range would overflow, @range is capped. > > + * > > + * NOTE: Historical use of randomize_range, which this replaces, presumed that > > + * @start was already page aligned. This assumption still holds. > > + * > > + * Return: A page aligned address within [start, start + range). On error, > > + * @start is returned. > > + */ > > +unsigned long > > +randomize_addr(unsigned long start, unsigned long range) > > Since we're changing other things about this, let's try to document > its behavior in its name too and call this "randomize_page" instead. Ack. Definitely more accurate. > If it requires a page-aligned value, we should probably also BUG_ON > it, or adjust the start too. merf. So, this whole series started from a suggested cleanup by William to s/get_random_int/get_random_long/. The current users have all been stable the way they are for a long time. Like pre-git long. So, if this is just a cleanup for those callers, I don't think we need to do more than we already are. However, if the intent is for this function to see wider use, then by all means, we need to handle start != PAGE_ALIGN(start). Do you have any new call sites in mind? thx, Jason. -- 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 Reply-To: kernel-hardening@lists.openwall.com Date: Sun, 31 Jul 2016 20:56:32 +0000 From: Jason Cooper Message-ID: <20160731205632.GY4541@io.lakedaemon.net> References: <20160728204730.27453-1-jason@lakedaemon.net> <20160730154244.403-1-jason@lakedaemon.net> <20160730154244.403-2-jason@lakedaemon.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [kernel-hardening] Re: [PATCH v2 1/7] random: Simplify API for random address requests To: Kees Cook Cc: "Roberts, William C" , Yann Droneaud , Linux-MM , LKML , "kernel-hardening@lists.openwall.com" , Russell King - ARM Linux , Andrew Morton , Theodore Ts'o , Arnd Bergmann , Greg KH , Catalin Marinas , Will Deacon , Ralf Baechle , "benh@kernel.crashing.org" , Paul Mackerras , Michael Ellerman , "David S. Miller" , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , Al Viro , Nick Kralevich , Jeffrey Vander Stoep , Daniel Cashman List-ID: On Sun, Jul 31, 2016 at 09:46:53AM -0700, Kees Cook wrote: > On Sat, Jul 30, 2016 at 8:42 AM, Jason Cooper wrote: > > To date, all callers of randomize_range() have set the length to 0, and > > check for a zero return value. For the current callers, the only way > > to get zero returned is if end <= start. Since they are all adding a > > constant to the start address, this is unnecessary. > > > > We can remove a bunch of needless checks by simplifying the API to do > > just what everyone wants, return an address between [start, start + > > range). > > > > While we're here, s/get_random_int/get_random_long/. No current call > > site is adversely affected by get_random_int(), since all current range > > requests are < UINT_MAX. However, we should match caller expectations > > to avoid coming up short (ha!) in the future. > > > > All current callers to randomize_range() chose to use the start address > > if randomize_range() failed. Therefore, we simplify things by just > > returning the start address on error. > > > > randomize_range() will be removed once all callers have been converted > > over to randomize_addr(). > > > > Signed-off-by: Jason Cooper > > --- > > Changes from v1: > > - Explicitly mention page_aligned start assumption (Yann Droneaud) > > - pick random pages vice random addresses (Yann Droneaud) > > - catch range=0 last > > > > drivers/char/random.c | 28 ++++++++++++++++++++++++++++ > > include/linux/random.h | 1 + > > 2 files changed, 29 insertions(+) > > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > index 0158d3bff7e5..3bedf69546d6 100644 > > --- a/drivers/char/random.c > > +++ b/drivers/char/random.c > > @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) > > return PAGE_ALIGN(get_random_int() % range + start); > > } > > > > +/** > > + * randomize_addr - Generate a random, page aligned address > > + * @start: The smallest acceptable address the caller will take. > > + * @range: The size of the area, starting at @start, within which the > > + * random address must fall. > > + * > > + * If @start + @range would overflow, @range is capped. > > + * > > + * NOTE: Historical use of randomize_range, which this replaces, presumed that > > + * @start was already page aligned. This assumption still holds. > > + * > > + * Return: A page aligned address within [start, start + range). On error, > > + * @start is returned. > > + */ > > +unsigned long > > +randomize_addr(unsigned long start, unsigned long range) > > Since we're changing other things about this, let's try to document > its behavior in its name too and call this "randomize_page" instead. Ack. Definitely more accurate. > If it requires a page-aligned value, we should probably also BUG_ON > it, or adjust the start too. merf. So, this whole series started from a suggested cleanup by William to s/get_random_int/get_random_long/. The current users have all been stable the way they are for a long time. Like pre-git long. So, if this is just a cleanup for those callers, I don't think we need to do more than we already are. However, if the intent is for this function to see wider use, then by all means, we need to handle start != PAGE_ALIGN(start). Do you have any new call sites in mind? thx, Jason.