| Message ID | 20160730154244.403-2-jason@lakedaemon.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 30, 2016 at 8:42 AM, 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 < UINT_MAX. However, we should match caller expectations > to avoid coming up short (ha!) in the future. > > All current callers to randomize_range() chose to use the start address > if randomize_range() failed. Therefore, we simplify things by just > returning the start address on error. > > randomize_range() will be removed once all callers have been converted > over to randomize_addr(). > > Signed-off-by: Jason Cooper <jason@lakedaemon.net> > --- > Changes from v1: > - Explicitly mention page_aligned start assumption (Yann Droneaud) > - pick random pages vice random addresses (Yann Droneaud) > - catch range=0 last > > drivers/char/random.c | 28 ++++++++++++++++++++++++++++ > include/linux/random.h | 1 + > 2 files changed, 29 insertions(+) > > diff --git a/drivers/char/random.c b/drivers/char/random.c > index 0158d3bff7e5..3bedf69546d6 100644 > --- a/drivers/char/random.c > +++ b/drivers/char/random.c > @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) > return PAGE_ALIGN(get_random_int() % range + start); > } > > +/** > + * randomize_addr - Generate a random, page aligned address > + * @start: The smallest acceptable address the caller will take. > + * @range: The size of the area, starting at @start, within which the > + * random address must fall. > + * > + * If @start + @range would overflow, @range is capped. > + * > + * NOTE: Historical use of randomize_range, which this replaces, presumed that > + * @start was already page aligned. This assumption still holds. > + * > + * Return: A page aligned address within [start, start + range). On error, > + * @start is returned. > + */ > +unsigned long > +randomize_addr(unsigned long start, unsigned long range) Since we're changing other things about this, let's try to document its behavior in its name too and call this "randomize_page" instead. If it requires a page-aligned value, we should probably also BUG_ON it, or adjust the start too. -Kees > +{ > + if (start > ULONG_MAX - range) > + range = ULONG_MAX - start; > + > + range >>= PAGE_SHIFT; > + > + if (range == 0) > + return start; > + > + return start + (get_random_long() % range << PAGE_SHIFT); > +} > + > /* Interface for in-kernel drivers of true hardware RNGs. > * Those devices may produce endless random bits and will be throttled > * when our pool is full. > diff --git a/include/linux/random.h b/include/linux/random.h > index e47e533742b5..f1ca2fa4c071 100644 > --- a/include/linux/random.h > +++ b/include/linux/random.h > @@ -35,6 +35,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 Sun, Jul 31, 2016 at 09:46:53AM -0700, Kees Cook wrote: > On Sat, Jul 30, 2016 at 8:42 AM, 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 < UINT_MAX. However, we should match caller expectations > > to avoid coming up short (ha!) in the future. > > > > All current callers to randomize_range() chose to use the start address > > if randomize_range() failed. Therefore, we simplify things by just > > returning the start address on error. > > > > randomize_range() will be removed once all callers have been converted > > over to randomize_addr(). > > > > Signed-off-by: Jason Cooper <jason@lakedaemon.net> > > --- > > Changes from v1: > > - Explicitly mention page_aligned start assumption (Yann Droneaud) > > - pick random pages vice random addresses (Yann Droneaud) > > - catch range=0 last > > > > drivers/char/random.c | 28 ++++++++++++++++++++++++++++ > > include/linux/random.h | 1 + > > 2 files changed, 29 insertions(+) > > > > diff --git a/drivers/char/random.c b/drivers/char/random.c > > index 0158d3bff7e5..3bedf69546d6 100644 > > --- a/drivers/char/random.c > > +++ b/drivers/char/random.c > > @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) > > return PAGE_ALIGN(get_random_int() % range + start); > > } > > > > +/** > > + * randomize_addr - Generate a random, page aligned address > > + * @start: The smallest acceptable address the caller will take. > > + * @range: The size of the area, starting at @start, within which the > > + * random address must fall. > > + * > > + * If @start + @range would overflow, @range is capped. > > + * > > + * NOTE: Historical use of randomize_range, which this replaces, presumed that > > + * @start was already page aligned. This assumption still holds. > > + * > > + * Return: A page aligned address within [start, start + range). On error, > > + * @start is returned. > > + */ > > +unsigned long > > +randomize_addr(unsigned long start, unsigned long range) > > Since we're changing other things about this, let's try to document > its behavior in its name too and call this "randomize_page" instead. Ack. Definitely more accurate. > If it requires a page-aligned value, we should probably also BUG_ON > it, or adjust the start too. merf. So, this whole series started from a suggested cleanup by William to s/get_random_int/get_random_long/. The current users have all been stable the way they are for a long time. Like pre-git long. So, if this is just a cleanup for those callers, I don't think we need to do more than we already are. However, if the intent is for this function to see wider use, then by all means, we need to handle start != PAGE_ALIGN(start). Do you have any new call sites in mind? thx, Jason.
On Sun, Jul 31, 2016 at 1:56 PM, Jason Cooper <jason@lakedaemon.net> wrote: > On Sun, Jul 31, 2016 at 09:46:53AM -0700, Kees Cook wrote: >> On Sat, Jul 30, 2016 at 8:42 AM, 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 < UINT_MAX. However, we should match caller expectations >> > to avoid coming up short (ha!) in the future. >> > >> > All current callers to randomize_range() chose to use the start address >> > if randomize_range() failed. Therefore, we simplify things by just >> > returning the start address on error. >> > >> > randomize_range() will be removed once all callers have been converted >> > over to randomize_addr(). >> > >> > Signed-off-by: Jason Cooper <jason@lakedaemon.net> >> > --- >> > Changes from v1: >> > - Explicitly mention page_aligned start assumption (Yann Droneaud) >> > - pick random pages vice random addresses (Yann Droneaud) >> > - catch range=0 last >> > >> > drivers/char/random.c | 28 ++++++++++++++++++++++++++++ >> > include/linux/random.h | 1 + >> > 2 files changed, 29 insertions(+) >> > >> > diff --git a/drivers/char/random.c b/drivers/char/random.c >> > index 0158d3bff7e5..3bedf69546d6 100644 >> > --- a/drivers/char/random.c >> > +++ b/drivers/char/random.c >> > @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) >> > return PAGE_ALIGN(get_random_int() % range + start); >> > } >> > >> > +/** >> > + * randomize_addr - Generate a random, page aligned address >> > + * @start: The smallest acceptable address the caller will take. >> > + * @range: The size of the area, starting at @start, within which the >> > + * random address must fall. >> > + * >> > + * If @start + @range would overflow, @range is capped. >> > + * >> > + * NOTE: Historical use of randomize_range, which this replaces, presumed that >> > + * @start was already page aligned. This assumption still holds. >> > + * >> > + * Return: A page aligned address within [start, start + range). On error, >> > + * @start is returned. >> > + */ >> > +unsigned long >> > +randomize_addr(unsigned long start, unsigned long range) >> >> Since we're changing other things about this, let's try to document >> its behavior in its name too and call this "randomize_page" instead. > > Ack. Definitely more accurate. > >> If it requires a page-aligned value, we should probably also BUG_ON >> it, or adjust the start too. > > merf. So, this whole series started from a suggested cleanup by William > to s/get_random_int/get_random_long/. > > The current users have all been stable the way they are for a long time. > Like pre-git long. So, if this is just a cleanup for those callers, I > don't think we need to do more than we already are. > > However, if the intent is for this function to see wider use, then by > all means, we need to handle start != PAGE_ALIGN(start). > > Do you have any new call sites in mind? I have no new call sites in mind, but it seems safe to add a BUG_ON to verify we don't gain callers that don't follow the correct expectations. (Or maybe WARN and return start.) -Kees
Hi Kees, On Mon, Aug 01, 2016 at 12:47:59PM -0700, Kees Cook wrote: > On Sun, Jul 31, 2016 at 1:56 PM, Jason Cooper <jason@lakedaemon.net> wrote: > > On Sun, Jul 31, 2016 at 09:46:53AM -0700, Kees Cook wrote: > >> On Sat, Jul 30, 2016 at 8:42 AM, 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 < UINT_MAX. However, we should match caller expectations > >> > to avoid coming up short (ha!) in the future. > >> > > >> > All current callers to randomize_range() chose to use the start address > >> > if randomize_range() failed. Therefore, we simplify things by just > >> > returning the start address on error. > >> > > >> > randomize_range() will be removed once all callers have been converted > >> > over to randomize_addr(). > >> > > >> > Signed-off-by: Jason Cooper <jason@lakedaemon.net> > >> > --- > >> > Changes from v1: > >> > - Explicitly mention page_aligned start assumption (Yann Droneaud) > >> > - pick random pages vice random addresses (Yann Droneaud) > >> > - catch range=0 last > >> > > >> > drivers/char/random.c | 28 ++++++++++++++++++++++++++++ > >> > include/linux/random.h | 1 + > >> > 2 files changed, 29 insertions(+) > >> > > >> > diff --git a/drivers/char/random.c b/drivers/char/random.c > >> > index 0158d3bff7e5..3bedf69546d6 100644 > >> > --- a/drivers/char/random.c > >> > +++ b/drivers/char/random.c > >> > @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) > >> > return PAGE_ALIGN(get_random_int() % range + start); > >> > } > >> > > >> > +/** > >> > + * randomize_addr - Generate a random, page aligned address > >> > + * @start: The smallest acceptable address the caller will take. > >> > + * @range: The size of the area, starting at @start, within which the > >> > + * random address must fall. > >> > + * > >> > + * If @start + @range would overflow, @range is capped. > >> > + * > >> > + * NOTE: Historical use of randomize_range, which this replaces, presumed that > >> > + * @start was already page aligned. This assumption still holds. > >> > + * > >> > + * Return: A page aligned address within [start, start + range). On error, > >> > + * @start is returned. > >> > + */ > >> > +unsigned long > >> > +randomize_addr(unsigned long start, unsigned long range) > >> > >> Since we're changing other things about this, let's try to document > >> its behavior in its name too and call this "randomize_page" instead. > > > > Ack. Definitely more accurate. > > > >> If it requires a page-aligned value, we should probably also BUG_ON > >> it, or adjust the start too. > > > > merf. So, this whole series started from a suggested cleanup by William > > to s/get_random_int/get_random_long/. > > > > The current users have all been stable the way they are for a long time. > > Like pre-git long. So, if this is just a cleanup for those callers, I > > don't think we need to do more than we already are. > > > > However, if the intent is for this function to see wider use, then by > > all means, we need to handle start != PAGE_ALIGN(start). > > > > Do you have any new call sites in mind? > > I have no new call sites in mind, but it seems safe to add a BUG_ON to > verify we don't gain callers that don't follow the correct > expectations. (Or maybe WARN and return start.) No, I think BUG_ON is appropriate. afaict, the only time this will be encountered is during the development process. thx, Jason.
Jason Cooper <jason@lakedaemon.net> writes: > On Mon, Aug 01, 2016 at 12:47:59PM -0700, Kees Cook wrote: >> On Sun, Jul 31, 2016 at 1:56 PM, Jason Cooper <jason@lakedaemon.net> wrote: >> >> I have no new call sites in mind, but it seems safe to add a BUG_ON to >> verify we don't gain callers that don't follow the correct >> expectations. (Or maybe WARN and return start.) > > No, I think BUG_ON is appropriate. afaict, the only time this will be > encountered is during the development process. Unless it's not. Why crash someone's system when you could just page align the value you're given? cheers
On Tue, Aug 02, 2016 at 01:35:13PM +1000, Michael Ellerman wrote: > Jason Cooper <jason@lakedaemon.net> writes: > > On Mon, Aug 01, 2016 at 12:47:59PM -0700, Kees Cook wrote: > >> On Sun, Jul 31, 2016 at 1:56 PM, Jason Cooper <jason@lakedaemon.net> wrote: > >> > >> I have no new call sites in mind, but it seems safe to add a BUG_ON to > >> verify we don't gain callers that don't follow the correct > >> expectations. (Or maybe WARN and return start.) > > > > No, I think BUG_ON is appropriate. afaict, the only time this will be > > encountered is during the development process. > > Unless it's not. > > Why crash someone's system when you could just page align the value > you're given? Ack, v3 on it's way. thx, Jason.
diff --git a/drivers/char/random.c b/drivers/char/random.c index 0158d3bff7e5..3bedf69546d6 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -1840,6 +1840,34 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len) return PAGE_ALIGN(get_random_int() % range + start); } +/** + * randomize_addr - Generate a random, page aligned address + * @start: The smallest acceptable address the caller will take. + * @range: The size of the area, starting at @start, within which the + * random address must fall. + * + * If @start + @range would overflow, @range is capped. + * + * NOTE: Historical use of randomize_range, which this replaces, presumed that + * @start was already page aligned. This assumption still holds. + * + * Return: A page aligned address within [start, start + range). On error, + * @start is returned. + */ +unsigned long +randomize_addr(unsigned long start, unsigned long range) +{ + if (start > ULONG_MAX - range) + range = ULONG_MAX - start; + + range >>= PAGE_SHIFT; + + if (range == 0) + return start; + + return start + (get_random_long() % range << PAGE_SHIFT); +} + /* Interface for in-kernel drivers of true hardware RNGs. * Those devices may produce endless random bits and will be throttled * when our pool is full. diff --git a/include/linux/random.h b/include/linux/random.h index e47e533742b5..f1ca2fa4c071 100644 --- a/include/linux/random.h +++ b/include/linux/random.h @@ -35,6 +35,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 < UINT_MAX. However, we should match caller expectations to avoid coming up short (ha!) in the future. All current callers to randomize_range() chose to use the start address if randomize_range() failed. Therefore, we simplify things by just returning the start address on error. randomize_range() will be removed once all callers have been converted over to randomize_addr(). Signed-off-by: Jason Cooper <jason@lakedaemon.net> --- Changes from v1: - Explicitly mention page_aligned start assumption (Yann Droneaud) - pick random pages vice random addresses (Yann Droneaud) - catch range=0 last drivers/char/random.c | 28 ++++++++++++++++++++++++++++ include/linux/random.h | 1 + 2 files changed, 29 insertions(+)