Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450347 > unrolled thread

[RFC patch 1/6] random: Simplify API for random address requests

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC patch 1/6] random: Simplify API for random address requests Jason Cooper <jason@lakedaemon.net> - 2016-07-26 05:10 +0200
    Re: [RFC patch 1/6] random: Simplify API for random address requests Jason Cooper <jason@lakedaemon.net> - 2016-07-26 05:40 +0200
      Re: [RFC patch 1/6] random: Simplify API for random address requests Kees Cook <keescook@chromium.org> - 2016-07-26 06:50 +0200
        Re: [RFC patch 1/6] random: Simplify API for random address requests Jason Cooper <jason@lakedaemon.net> - 2016-07-26 19:10 +0200
          Re: [RFC patch 1/6] random: Simplify API for random address requests Kees Cook <keescook@chromium.org> - 2016-07-26 19:10 +0200
            Re: [RFC patch 1/6] random: Simplify API for random address requests Jason Cooper <jason@lakedaemon.net> - 2016-07-28 21:10 +0200
      RE: [RFC patch 1/6] random: Simplify API for random address requests "Roberts, William C" <william.c.roberts@intel.com> - 2016-07-26 19:40 +0200
    Re: [RFC patch 1/6] random: Simplify API for random address requests Kees Cook <keescook@chromium.org> - 2016-07-26 06:50 +0200
      Re: [RFC patch 1/6] random: Simplify API for random address requests Jason Cooper <jason@lakedaemon.net> - 2016-07-26 18:00 +0200
        Re: [RFC patch 1/6] random: Simplify API for random address requests Kees Cook <keescook@chromium.org> - 2016-07-26 18:50 +0200
    Re: [kernel-hardening] [RFC patch 1/6] random: Simplify API for  random address requests Yann Droneaud <ydroneaud@opteya.com> - 2016-07-27 16:30 +0200

#1450347 — [RFC patch 1/6] random: Simplify API for random address requests

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
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);
 }
 
 /* Interface for in-kernel drivers of true hardware RNGs.
diff --git a/include/linux/random.h b/include/linux/random.h
index e47e533742b5..1ad877a98186 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -34,7 +34,7 @@ extern const struct file_operations random_fops, urandom_fops;
 
 unsigned int get_random_int(void);
 unsigned long get_random_long(void);
-unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
+unsigned long randomize_addr(unsigned long start, unsigned long range);
 
 u32 prandom_u32(void);
 void prandom_bytes(void *buf, size_t nbytes);
-- 
2.9.2

[toc] | [next] | [standalone]

#1450350

All,

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
> 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);

sorry,

Jason.

>  
>  /* Interface for in-kernel drivers of true hardware RNGs.
> diff --git a/include/linux/random.h b/include/linux/random.h
> index e47e533742b5..1ad877a98186 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -34,7 +34,7 @@ extern const struct file_operations random_fops, urandom_fops;
>  
>  unsigned int get_random_int(void);
>  unsigned long get_random_long(void);
> -unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
> +unsigned long randomize_addr(unsigned long start, unsigned long range);
>  
>  u32 prandom_u32(void);
>  void prandom_bytes(void *buf, size_t nbytes);
> -- 
> 2.9.2
> 

[toc] | [prev] | [next] | [standalone]

#1450374

On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> All,
>
> 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
>> 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, and it
should be very clear if the range is inclusive or exclusive.  start =
0, range = 4096. does this mean 1 page, or 2 pages possible?

-Kees

>
> sorry,
>
> Jason.
>
>>
>>  /* Interface for in-kernel drivers of true hardware RNGs.
>> diff --git a/include/linux/random.h b/include/linux/random.h
>> index e47e533742b5..1ad877a98186 100644
>> --- a/include/linux/random.h
>> +++ b/include/linux/random.h
>> @@ -34,7 +34,7 @@ extern const struct file_operations random_fops, urandom_fops;
>>
>>  unsigned int get_random_int(void);
>>  unsigned long get_random_long(void);
>> -unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
>> +unsigned long randomize_addr(unsigned long start, unsigned long range);
>>
>>  u32 prandom_u32(void);
>>  void prandom_bytes(void *buf, size_t nbytes);
>> --
>> 2.9.2
>>



-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1450693

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.

if (range == 0 || ULONG_MAX - range < start)
	return 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.

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1450694

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

[toc] | [prev] | [next] | [standalone]

#1452059

On Tue, Jul 26, 2016 at 10:07:22AM -0700, Kees Cook wrote:
> On Tue, Jul 26, 2016 at 10:00 AM, Jason Cooper <jason@lakedaemon.net> wrote:
...
> > 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

yes, will do.

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1450712

> -----Original Message-----
> From: Jason Cooper [mailto:jason@lakedaemon.net]
> Sent: Monday, July 25, 2016 8:31 PM
> To: Roberts, William C <william.c.roberts@intel.com>; linux-
> mm@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> hardening@lists.openwall.com
> Cc: linux@arm.linux.org.uk; akpm@linux-foundation.org;
> keescook@chromium.org; tytso@mit.edu; arnd@arndb.de;
> gregkh@linuxfoundation.org; catalin.marinas@arm.com; will.deacon@arm.com;
> ralf@linux-mips.org; benh@kernel.crashing.org; paulus@samba.org;
> mpe@ellerman.id.au; davem@davemloft.net; tglx@linutronix.de;
> mingo@redhat.com; hpa@zytor.com; x86@kernel.org; viro@zeniv.linux.org.uk;
> nnk@google.com; jeffv@google.com; alyzyn@android.com;
> dcashman@android.com
> Subject: Re: [RFC patch 1/6] random: Simplify API for random address requests
> 
> All,
> 
> 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 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);
> 
> sorry,

Yeah that looks better. I had a similar intended set of changes locally, because of the issues Jason pointed out.
I ended up in the old case where if end - start == len it returns 0 instead of start. Jason's change is better though :-P

> 
> Jason.
> 
> >
> >  /* Interface for in-kernel drivers of true hardware RNGs.
> > diff --git a/include/linux/random.h b/include/linux/random.h index
> > e47e533742b5..1ad877a98186 100644
> > --- a/include/linux/random.h
> > +++ b/include/linux/random.h
> > @@ -34,7 +34,7 @@ extern const struct file_operations random_fops,
> > urandom_fops;
> >
> >  unsigned int get_random_int(void);
> >  unsigned long get_random_long(void);
> > -unsigned long randomize_range(unsigned long start, unsigned long end,
> > unsigned long len);
> > +unsigned long randomize_addr(unsigned long start, unsigned long
> > +range);
> >
> >  u32 prandom_u32(void);
> >  void prandom_bytes(void *buf, size_t nbytes);
> > --
> > 2.9.2
> >

[toc] | [prev] | [next] | [standalone]

#1450369

On Mon, Jul 25, 2016 at 8:01 PM, Jason Cooper <jason@lakedaemon.net> 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
> 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)

Also, this series isn't bisectable since randomize_range gets removed
here before the callers are updated. Perhaps add a macro that calls
randomize_addr with a BUG_ON for len != 0? (And then remove it in the
last patch?)

-Kees

>  {
> -       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);
>  }
>
>  /* Interface for in-kernel drivers of true hardware RNGs.
> diff --git a/include/linux/random.h b/include/linux/random.h
> index e47e533742b5..1ad877a98186 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -34,7 +34,7 @@ extern const struct file_operations random_fops, urandom_fops;
>
>  unsigned int get_random_int(void);
>  unsigned long get_random_long(void);
> -unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
> +unsigned long randomize_addr(unsigned long start, unsigned long range);
>
>  u32 prandom_u32(void);
>  void prandom_bytes(void *buf, size_t nbytes);
> --
> 2.9.2
>



-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1450661

On Mon, Jul 25, 2016 at 09:44:27PM -0700, Kees Cook wrote:
> On Mon, Jul 25, 2016 at 8:01 PM, Jason Cooper <jason@lakedaemon.net> 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
> > 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)
> 
> Also, this series isn't bisectable since randomize_range gets removed
> here before the callers are updated. Perhaps add a macro that calls
> randomize_addr with a BUG_ON for len != 0? (And then remove it in the
> last patch?)

No, I was thinking just add randomize_addr() in the first patch, convert
all the callers, then the last patch would remove randomize_range().

That way the last patch can be a cleanup in a later merge window if
needed.

thx,

Jason.

[toc] | [prev] | [next] | [standalone]

#1450683

On Tue, Jul 26, 2016 at 8:55 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> On Mon, Jul 25, 2016 at 09:44:27PM -0700, Kees Cook wrote:
>> On Mon, Jul 25, 2016 at 8:01 PM, Jason Cooper <jason@lakedaemon.net> 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
>> > 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)
>>
>> Also, this series isn't bisectable since randomize_range gets removed
>> here before the callers are updated. Perhaps add a macro that calls
>> randomize_addr with a BUG_ON for len != 0? (And then remove it in the
>> last patch?)
>
> No, I was thinking just add randomize_addr() in the first patch, convert
> all the callers, then the last patch would remove randomize_range().
>
> That way the last patch can be a cleanup in a later merge window if
> needed.

That works too! :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1451301 — Re: [kernel-hardening] [RFC patch 1/6] random: Simplify API for random address requests

Hi,

Le mardi 26 juillet 2016 à 03:01 +0000, Jason Cooper a écrit :
> 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.
> 

I agree.

> 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].
> 

I agree.

For the record:

http://lkml.kernel.org/r/cover.1390770607.git.ydroneaud@opteya.com


> 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 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);
>  }
>  
>  /* Interface for in-kernel drivers of true hardware RNGs.
> diff --git a/include/linux/random.h b/include/linux/random.h
> index e47e533742b5..1ad877a98186 100644
> --- a/include/linux/random.h
> +++ b/include/linux/random.h
> @@ -34,7 +34,7 @@ extern const struct file_operations random_fops,
> urandom_fops;
>  
>  unsigned int get_random_int(void);
>  unsigned long get_random_long(void);
> -unsigned long randomize_range(unsigned long start, unsigned long
> end, unsigned long len);
> +unsigned long randomize_addr(unsigned long start, unsigned long
> range);
>  
>  u32 prandom_u32(void);
>  void prandom_bytes(void *buf, size_t nbytes);

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web