| Message ID | 20160726030201.6775-1-jason@lakedaemon.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >>
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 >
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.
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
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.
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.
> -----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 > >
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);
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.
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);
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(-)