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

[Roberts, William C]

> -----Original Message-----
> From: Jason Cooper [mailto:jason@xxxxxxxxxxxxxx]
> Sent: Monday, July 25, 2016 8:31 PM
> To: Roberts, William C <william.c.roberts@xxxxxxxxx>; linux-
> mm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kernel-
> hardening@xxxxxxxxxxxxxxxxxx
> Cc: linux@xxxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
> keescook@xxxxxxxxxxxx; tytso@xxxxxxx; arnd@xxxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; catalin.marinas@xxxxxxx; will.deacon@xxxxxxx;
> ralf@xxxxxxxxxxxxxx; benh@xxxxxxxxxxxxxxxxxxx; paulus@xxxxxxxxx;
> mpe@xxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> mingo@xxxxxxxxxx; hpa@xxxxxxxxx; x86@xxxxxxxxxx; viro@xxxxxxxxxxxxxxxxxx;
> nnk@xxxxxxxxxx; jeffv@xxxxxxxxxx; alyzyn@xxxxxxxxxxx;
> dcashman@xxxxxxxxxxx
> 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@xxxxxxxxxxxxxx>
> > ---
> >  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
> >