From: Kees Cook <keescook@chromium.org> To: Jason Cooper <jason@lakedaemon.net> Cc: "Roberts, William C" <william.c.roberts@intel.com>, linux-mm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, "kernel-hardening@lists.openwall.com" <kernel-hardening@lists.openwall.com>, Russell King - ARM Linux <linux@arm.linux.org.uk>, Andrew Morton <akpm@linux-foundation.org>, "Theodore Ts'o" <tytso@mit.edu>, Arnd Bergmann <arnd@arndb.de>, Greg KH <gregkh@linuxfoundation.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Ralf Baechle <ralf@linux-mips.org>, "benh@kernel.crashing.org" <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Michael Ellerman <mpe@ellerman.id.au>, "David S. Miller" <davem@davemloft.net>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>, Al Viro <viro@zeniv.linux.org.uk>, Nick Kralevich <nnk@google.com>, Jeffrey Vander Stoep <jeffv@google.com>, alyzyn@android.com, Daniel Cashman <dcashman@android.com> Subject: Re: [RFC patch 1/6] random: Simplify API for random address requests Date: Tue, 26 Jul 2016 10:07:22 -0700 [thread overview] Message-ID: <CAGXu5jLT-pZc_67UeTJj-dCsRJteT2CCwaviGOKCXHnL8t-URw@mail.gmail.com> (raw) In-Reply-To: <20160726170052.GI4541@io.lakedaemon.net> On Tue, Jul 26, 2016 at 10:00 AM, Jason Cooper <jason@lakedaemon.net> wrote: > Hi Kees, > > On Mon, Jul 25, 2016 at 09:39:58PM -0700, Kees Cook wrote: >> On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper <jason@lakedaemon.net> wrote: >> > On Tue, Jul 26, 2016 at 03:01:55AM +0000, 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 < MAX_UINT. However, we should match caller expectations > > merf. UINT_MAX. > >> >> to avoid coming up short (ha!) in the future. >> >> >> >> Signed-off-by: Jason Cooper <jason@lakedaemon.net> >> >> --- >> >> drivers/char/random.c | 17 ++++------------- >> >> include/linux/random.h | 2 +- >> >> 2 files changed, 5 insertions(+), 14 deletions(-) >> >> >> >> diff --git a/drivers/char/random.c b/drivers/char/random.c >> >> index 0158d3bff7e5..1251cb2cbab2 100644 >> >> --- a/drivers/char/random.c >> >> +++ b/drivers/char/random.c >> >> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void) >> >> EXPORT_SYMBOL(get_random_long); >> >> >> >> /* >> >> - * randomize_range() returns a start address such that >> >> - * >> >> - * [...... <range> .....] >> >> - * start end >> >> - * >> >> - * a <range> with size "len" starting at the return value is inside in the >> >> - * area defined by [start, end], but is otherwise randomized. >> >> + * randomize_addr() returns a page aligned address within [start, start + >> >> + * range] >> >> */ >> >> unsigned long >> >> -randomize_range(unsigned long start, unsigned long end, unsigned long len) >> >> +randomize_addr(unsigned long start, unsigned long range) >> >> { >> >> - unsigned long range = end - len - start; >> >> - >> >> - if (end <= start + len) >> >> - return 0; >> >> - return PAGE_ALIGN(get_random_int() % range + start); >> >> + return PAGE_ALIGN(get_random_long() % range + start); >> >> } >> > >> > bah! old patch file. This should have been: >> > >> > if (range == 0) >> > return start; >> > else >> > return PAGE_ALIGN(get_random_long() % range + start); >> >> I think range should be limited to start + range < UINTMAX, > > ULONG_MAX? I agree. Heh, I am plagued by misspelling these constants, and yes, sorry, ULONG_MAX. :) > if (range == 0 || ULONG_MAX - range < start) > return start; Should it "abort" like this? I was thinking just cap the range, something like: if (range > ULONG_MAX - start) range = ULONG_MAX - start > else > return PAGE_ALIGN(get_random_long() % range + start); > > ? > >> and it should be very clear if the range is inclusive or exclusive. > > Sorry, I was reading the original comment, '[start, end]' with square > brackets denoting inclusive. > > Regardless, the math in randomize_range() was just undoing the math at > each of the call sites. This proposed change to randomize_addr() > doesn't alter the current state of affairs wrt inclusive, exclusive. > >> start = 0, range = 4096. does this mean 1 page, or 2 pages possible? > > ooh, good spot. What we have right now is [start, start + range), which > is matching previous behavior. But does not match the old comment, > [start, end]. It should have been [start, end). > > So, you're correct, I need to clarify this in the comments. Okay, cool. Thanks! I'm glad to have this clean-up. :) -Kees > > thx, > > Jason. -- Kees Cook Chrome OS & Brillo Security
WARNING: multiple messages have this Message-ID (diff)
From: Kees Cook <keescook@chromium.org> To: Jason Cooper <jason@lakedaemon.net> Cc: "Roberts, William C" <william.c.roberts@intel.com>, linux-mm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>, "kernel-hardening@lists.openwall.com" <kernel-hardening@lists.openwall.com>, Russell King - ARM Linux <linux@arm.linux.org.uk>, Andrew Morton <akpm@linux-foundation.org>, Theodore Ts'o <tytso@mit.edu>, Arnd Bergmann <arnd@arndb.de>, Greg KH <gregkh@linuxfoundation.org>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Ralf Baechle <ralf@linux-mips.org>, "benh@kernel.crashing.org" <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Michael Ellerman <mpe@ellerman.id.au>, "David S. Miller" <davem@davemloft.net>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>, "x86@kernel.org" <x86@kernel.org>, Al Viro <viro@zeniv.linux.org.uk>, Nick Kralevich <nnk@google.com>, Jeffrey Vander Stoep <jeffv@google.com>, alyzyn@android.com, Daniel Cashman <dcashman@android.com> Subject: [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests Date: Tue, 26 Jul 2016 10:07:22 -0700 [thread overview] Message-ID: <CAGXu5jLT-pZc_67UeTJj-dCsRJteT2CCwaviGOKCXHnL8t-URw@mail.gmail.com> (raw) In-Reply-To: <20160726170052.GI4541@io.lakedaemon.net> On Tue, Jul 26, 2016 at 10:00 AM, Jason Cooper <jason@lakedaemon.net> wrote: > Hi Kees, > > On Mon, Jul 25, 2016 at 09:39:58PM -0700, Kees Cook wrote: >> On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper <jason@lakedaemon.net> wrote: >> > On Tue, Jul 26, 2016 at 03:01:55AM +0000, 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 < MAX_UINT. However, we should match caller expectations > > merf. UINT_MAX. > >> >> to avoid coming up short (ha!) in the future. >> >> >> >> Signed-off-by: Jason Cooper <jason@lakedaemon.net> >> >> --- >> >> drivers/char/random.c | 17 ++++------------- >> >> include/linux/random.h | 2 +- >> >> 2 files changed, 5 insertions(+), 14 deletions(-) >> >> >> >> diff --git a/drivers/char/random.c b/drivers/char/random.c >> >> index 0158d3bff7e5..1251cb2cbab2 100644 >> >> --- a/drivers/char/random.c >> >> +++ b/drivers/char/random.c >> >> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void) >> >> EXPORT_SYMBOL(get_random_long); >> >> >> >> /* >> >> - * randomize_range() returns a start address such that >> >> - * >> >> - * [...... <range> .....] >> >> - * start end >> >> - * >> >> - * a <range> with size "len" starting at the return value is inside in the >> >> - * area defined by [start, end], but is otherwise randomized. >> >> + * randomize_addr() returns a page aligned address within [start, start + >> >> + * range] >> >> */ >> >> unsigned long >> >> -randomize_range(unsigned long start, unsigned long end, unsigned long len) >> >> +randomize_addr(unsigned long start, unsigned long range) >> >> { >> >> - unsigned long range = end - len - start; >> >> - >> >> - if (end <= start + len) >> >> - return 0; >> >> - return PAGE_ALIGN(get_random_int() % range + start); >> >> + return PAGE_ALIGN(get_random_long() % range + start); >> >> } >> > >> > bah! old patch file. This should have been: >> > >> > if (range == 0) >> > return start; >> > else >> > return PAGE_ALIGN(get_random_long() % range + start); >> >> I think range should be limited to start + range < UINTMAX, > > ULONG_MAX? I agree. Heh, I am plagued by misspelling these constants, and yes, sorry, ULONG_MAX. :) > if (range == 0 || ULONG_MAX - range < start) > return start; Should it "abort" like this? I was thinking just cap the range, something like: if (range > ULONG_MAX - start) range = ULONG_MAX - start > else > return PAGE_ALIGN(get_random_long() % range + start); > > ? > >> and it should be very clear if the range is inclusive or exclusive. > > Sorry, I was reading the original comment, '[start, end]' with square > brackets denoting inclusive. > > Regardless, the math in randomize_range() was just undoing the math at > each of the call sites. This proposed change to randomize_addr() > doesn't alter the current state of affairs wrt inclusive, exclusive. > >> start = 0, range = 4096. does this mean 1 page, or 2 pages possible? > > ooh, good spot. What we have right now is [start, start + range), which > is matching previous behavior. But does not match the old comment, > [start, end]. It should have been [start, end). > > So, you're correct, I need to clarify this in the comments. Okay, cool. Thanks! I'm glad to have this clean-up. :) -Kees > > thx, > > Jason. -- Kees Cook Chrome OS & Brillo Security
next prev parent reply other threads:[~2016-07-26 17:07 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-07-25 18:25 [PATCH] randomize_range: use random long instead of int william.c.roberts 2016-07-25 18:54 ` Kees Cook 2016-07-26 2:18 ` Jason Cooper 2016-07-26 3:01 ` [RFC patch 1/6] random: Simplify API for random address requests Jason Cooper 2016-07-26 3:01 ` [kernel-hardening] " Jason Cooper 2016-07-26 3:01 ` [RFC patch 2/6] x86: Use simpler " Jason Cooper 2016-07-26 3:01 ` [kernel-hardening] " Jason Cooper 2016-07-26 3:01 ` [RFC patch 3/6] ARM: " Jason Cooper 2016-07-26 3:01 ` [kernel-hardening] " Jason Cooper 2016-07-26 3:01 ` [RFC patch 4/6] arm64: " Jason Cooper 2016-07-26 3:01 ` [kernel-hardening] " Jason Cooper 2016-07-26 3:01 ` [RFC patch 5/6] tile: " Jason Cooper 2016-07-26 3:01 ` [kernel-hardening] " Jason Cooper 2016-07-26 3:02 ` [RFC patch 6/6] unicore32: " Jason Cooper 2016-07-26 3:02 ` [kernel-hardening] " Jason Cooper 2016-07-26 3:30 ` [RFC patch 1/6] random: Simplify " Jason Cooper 2016-07-26 3:30 ` [kernel-hardening] " Jason Cooper 2016-07-26 4:39 ` Kees Cook 2016-07-26 4:39 ` [kernel-hardening] " Kees Cook 2016-07-26 17:00 ` Jason Cooper 2016-07-26 17:00 ` [kernel-hardening] " Jason Cooper 2016-07-26 17:07 ` Kees Cook [this message] 2016-07-26 17:07 ` Kees Cook 2016-07-28 19:02 ` Jason Cooper 2016-07-28 19:02 ` [kernel-hardening] " Jason Cooper 2016-07-26 17:33 ` Roberts, William C 2016-07-26 17:33 ` [kernel-hardening] " Roberts, William C 2016-07-26 4:44 ` Kees Cook 2016-07-26 4:44 ` [kernel-hardening] " Kees Cook 2016-07-26 15:55 ` Jason Cooper 2016-07-26 15:55 ` [kernel-hardening] " Jason Cooper 2016-07-26 16:40 ` Kees Cook 2016-07-26 16:40 ` [kernel-hardening] " Kees Cook 2016-07-27 13:51 ` [kernel-hardening] " Yann Droneaud
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGXu5jLT-pZc_67UeTJj-dCsRJteT2CCwaviGOKCXHnL8t-URw@mail.gmail.com \ --to=keescook@chromium.org \ --cc=akpm@linux-foundation.org \ --cc=alyzyn@android.com \ --cc=arnd@arndb.de \ --cc=benh@kernel.crashing.org \ --cc=catalin.marinas@arm.com \ --cc=davem@davemloft.net \ --cc=dcashman@android.com \ --cc=gregkh@linuxfoundation.org \ --cc=hpa@zytor.com \ --cc=jason@lakedaemon.net \ --cc=jeffv@google.com \ --cc=kernel-hardening@lists.openwall.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=mingo@redhat.com \ --cc=mpe@ellerman.id.au \ --cc=nnk@google.com \ --cc=paulus@samba.org \ --cc=ralf@linux-mips.org \ --cc=tglx@linutronix.de \ --cc=tytso@mit.edu \ --cc=viro@zeniv.linux.org.uk \ --cc=will.deacon@arm.com \ --cc=william.c.roberts@intel.com \ --cc=x86@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.