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

[Jason Cooper]

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

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

[Jason Cooper]

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

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

[Roberts, William C]

> -Original Message-
> From: Jason Cooper [mailto:ja...@lakedaemon.net]
> Sent: Monday, July 25, 2016 8:31 PM
> To: Roberts, William C <william.c.robe...@intel.com>; linux-
> m...@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> harden...@lists.openwall.com
> Cc: li...@arm.linux.org.uk; a...@linux-foundation.org;
> keesc...@chromium.org; ty...@mit.edu; a...@arndb.de;
> gre...@linuxfoundation.org; catalin.mari...@arm.com; will.dea...@arm.com;
> r...@linux-mips.org; b...@kernel.crashing.org; pau...@samba.org;
> m...@ellerman.id.au; da...@davemloft.net; t...@linutronix.de;
> mi...@redhat.com; h...@zytor.com; x...@kernel.org; v...@zeniv.linux.org.uk;
> n...@google.com; je...@google.com; aly...@android.com;
> dcash...@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 +, 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 <ja...@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
> > - *
> > - *[..  .]
> > - *  start  end
> > - *
> > - * a  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
> >

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

[Roberts, William C]

> -Original Message-
> From: Jason Cooper [mailto:ja...@lakedaemon.net]
> Sent: Monday, July 25, 2016 8:31 PM
> To: Roberts, William C ; linux-
> m...@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-
> harden...@lists.openwall.com
> Cc: li...@arm.linux.org.uk; a...@linux-foundation.org;
> keesc...@chromium.org; ty...@mit.edu; a...@arndb.de;
> gre...@linuxfoundation.org; catalin.mari...@arm.com; will.dea...@arm.com;
> r...@linux-mips.org; b...@kernel.crashing.org; pau...@samba.org;
> m...@ellerman.id.au; da...@davemloft.net; t...@linutronix.de;
> mi...@redhat.com; h...@zytor.com; x...@kernel.org; v...@zeniv.linux.org.uk;
> n...@google.com; je...@google.com; aly...@android.com;
> dcash...@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 +, 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 
> > ---
> >  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
> > - *
> > - *[..  .]
> > - *  start  end
> > - *
> > - * a  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
> >

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

[Kees Cook]

On Tue, Jul 26, 2016 at 10:00 AM, Jason Cooper  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  wrote:
>> > On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
>> >> ---
>> >>  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
>> >> - *
>> >> - *[..  .]
>> >> - *  start  end
>> >> - *
>> >> - * a  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

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

[Kees Cook]

On Tue, Jul 26, 2016 at 10:00 AM, Jason Cooper  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  wrote:
>> > On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
>> >> ---
>> >>  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
>> >> - *
>> >> - *[..  .]
>> >> - *  start  end
>> >> - *
>> >> - * a  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

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

[Jason Cooper]

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  wrote:
> > On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
> >> ---
> >>  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
> >> - *
> >> - *[..  .]
> >> - *  start  end
> >> - *
> >> - * a  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.

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

[Jason Cooper]

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  wrote:
> > On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
> >> ---
> >>  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
> >> - *
> >> - *[..  .]
> >> - *  start  end
> >> - *
> >> - * a  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.

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

[Kees Cook]

On Tue, Jul 26, 2016 at 8:55 AM, Jason Cooper  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  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 
>> > ---
>> >  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
>> > - *
>> > - *[..  .]
>> > - *  start  end
>> > - *
>> > - * a  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

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

[Kees Cook]

On Tue, Jul 26, 2016 at 8:55 AM, Jason Cooper  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  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 
>> > ---
>> >  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
>> > - *
>> > - *[..  .]
>> > - *  start  end
>> > - *
>> > - * a  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

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

[Jason Cooper]

On Mon, Jul 25, 2016 at 09:44:27PM -0700, Kees Cook wrote:
> On Mon, Jul 25, 2016 at 8:01 PM, 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 
> > ---
> >  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
> > - *
> > - *[..  .]
> > - *  start  end
> > - *
> > - * a  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.

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

[Jason Cooper]

On Mon, Jul 25, 2016 at 09:44:27PM -0700, Kees Cook wrote:
> On Mon, Jul 25, 2016 at 8:01 PM, 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 
> > ---
> >  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
> > - *
> > - *[..  .]
> > - *  start  end
> > - *
> > - * a  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.

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

[Kees Cook]

On Mon, Jul 25, 2016 at 8:01 PM, 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 
> ---
>  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
> - *
> - *[..  .]
> - *  start  end
> - *
> - * a  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

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

[Kees Cook]

On Mon, Jul 25, 2016 at 8:01 PM, 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 
> ---
>  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
> - *
> - *[..  .]
> - *  start  end
> - *
> - * a  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

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

[Kees Cook]

On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper  wrote:
> All,
>
> On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
>> ---
>>  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
>> - *
>> - *[..  .]
>> - *  start  end
>> - *
>> - * a  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

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

[Kees Cook]

On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper  wrote:
> All,
>
> On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
>> ---
>>  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
>> - *
>> - *[..  .]
>> - *  start  end
>> - *
>> - * a  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

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

[Jason Cooper]

All,

On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
> ---
>  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
> - *
> - *[..  .]
> - *  start  end
> - *
> - * a  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
> 

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

[Jason Cooper]

All,

On Tue, Jul 26, 2016 at 03:01:55AM +, 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 
> ---
>  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
> - *
> - *[..  .]
> - *  start  end
> - *
> - * a  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
>