All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] randomize_range: use random long instead of int
@ 2016-07-25 18:25 william.c.roberts
  2016-07-25 18:54 ` Kees Cook
                   ` (2 more replies)
  0 siblings, 3 replies; 34+ messages in thread
From: william.c.roberts @ 2016-07-25 18:25 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: linux, akpm, keescook, tytso, arnd, gregkh, catalin.marinas,
	will.deacon, ralf, benh, paulus, mpe, davem, tglx, mingo, hpa,
	x86, viro, nnk, jeffv, salyzyn, dcashman, William Roberts

From: William Roberts <william.c.roberts@intel.com>

Use a long when generating the random range rather than
an int. This will produce better random distributions as
well as matching all the types at hand.

Signed-off-by: William Roberts <william.c.roberts@intel.com>
---
 drivers/char/random.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 0158d3b..bbf11b5 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1837,7 +1837,8 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len)
 
 	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.
-- 
1.9.1

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] randomize_range: use random long instead of int
  2016-07-25 18:25 [PATCH] randomize_range: use random long instead of int william.c.roberts
@ 2016-07-25 18:54 ` Kees Cook
  2016-07-26  2:18 ` Jason Cooper
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
  2 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-25 18:54 UTC (permalink / raw)
  To: Roberts, William C, Andrew Morton
  Cc: linux-mm, LKML, Russell King - ARM Linux, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, Mark Salyzyn,
	Daniel Cashman

On Mon, Jul 25, 2016 at 11:25 AM,  <william.c.roberts@intel.com> wrote:
> From: William Roberts <william.c.roberts@intel.com>
>
> Use a long when generating the random range rather than
> an int. This will produce better random distributions as
> well as matching all the types at hand.
>
> Signed-off-by: William Roberts <william.c.roberts@intel.com>

I agree, this is what I pointed out back in Feb:
https://lkml.org/lkml/2016/2/4/854

Acked-by: Kees Cook <keescook@chromium.org>

Andrew, can you pick this up for 4.8?

-Kees

> ---
>  drivers/char/random.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 0158d3b..bbf11b5 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1837,7 +1837,8 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len)
>
>         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.
> --
> 1.9.1
>



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] randomize_range: use random long instead of int
  2016-07-25 18:25 [PATCH] randomize_range: use random long instead of int william.c.roberts
  2016-07-25 18:54 ` Kees Cook
@ 2016-07-26  2:18 ` Jason Cooper
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
  2 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  2:18 UTC (permalink / raw)
  To: william.c.roberts
  Cc: linux-mm, linux-kernel, linux, akpm, keescook, tytso, arnd,
	gregkh, catalin.marinas, will.deacon, ralf, benh, paulus, mpe,
	davem, tglx, mingo, hpa, x86, viro, nnk, jeffv, salyzyn,
	dcashman

Hi William, Kees,

On Mon, Jul 25, 2016 at 11:25:41AM -0700, william.c.roberts@intel.com wrote:
> From: William Roberts <william.c.roberts@intel.com>
> 
> Use a long when generating the random range rather than
> an int. This will produce better random distributions as
> well as matching all the types at hand.
> 
> Signed-off-by: William Roberts <william.c.roberts@intel.com>
> ---
>  drivers/char/random.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Upon further review, I think we should dig into this a little bit
deeper.  Standby, I'll post an RFC series shortly.

thx,

Jason.

> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 0158d3b..bbf11b5 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1837,7 +1837,8 @@ randomize_range(unsigned long start, unsigned long end, unsigned long len)
>  
>  	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.
> -- 
> 1.9.1
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-25 18:25 [PATCH] randomize_range: use random long instead of int william.c.roberts
@ 2016-07-26  3:01   ` Jason Cooper
  2016-07-26  2:18 ` Jason Cooper
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
  2 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

To date, all callers of randomize_range() have set the length to 0, and
check for a zero return value.  For the current callers, the only way
to get zero returned is if end <= start.  Since they are all adding a
constant to the start address, this is unnecessary.

We can remove a bunch of needless checks by simplifying the API to do
just what everyone wants, return an address between [start, start +
range].

While we're here, s/get_random_int/get_random_long/.  No current call
site is adversely affected by get_random_int(), since all current range
requests are < MAX_UINT.  However, we should match caller expectations
to avoid coming up short (ha!) in the future.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 drivers/char/random.c  | 17 ++++-------------
 include/linux/random.h |  2 +-
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 0158d3bff7e5..1251cb2cbab2 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
 EXPORT_SYMBOL(get_random_long);
 
 /*
- * randomize_range() returns a start address such that
- *
- *    [...... <range> .....]
- *  start                  end
- *
- * a <range> with size "len" starting at the return value is inside in the
- * area defined by [start, end], but is otherwise randomized.
+ * randomize_addr() returns a page aligned address within [start, start +
+ * range]
  */
 unsigned long
-randomize_range(unsigned long start, unsigned long end, unsigned long len)
+randomize_addr(unsigned long start, unsigned long range)
 {
-	unsigned long range = end - len - start;
-
-	if (end <= start + len)
-		return 0;
-	return PAGE_ALIGN(get_random_int() % range + start);
+	return PAGE_ALIGN(get_random_long() % range + start);
 }
 
 /* Interface for in-kernel drivers of true hardware RNGs.
diff --git a/include/linux/random.h b/include/linux/random.h
index e47e533742b5..1ad877a98186 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -34,7 +34,7 @@ extern const struct file_operations random_fops, urandom_fops;
 
 unsigned int get_random_int(void);
 unsigned long get_random_long(void);
-unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
+unsigned long randomize_addr(unsigned long start, unsigned long range);
 
 u32 prandom_u32(void);
 void prandom_bytes(void *buf, size_t nbytes);
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26  3:01   ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

To date, all callers of randomize_range() have set the length to 0, and
check for a zero return value.  For the current callers, the only way
to get zero returned is if end <= start.  Since they are all adding a
constant to the start address, this is unnecessary.

We can remove a bunch of needless checks by simplifying the API to do
just what everyone wants, return an address between [start, start +
range].

While we're here, s/get_random_int/get_random_long/.  No current call
site is adversely affected by get_random_int(), since all current range
requests are < MAX_UINT.  However, we should match caller expectations
to avoid coming up short (ha!) in the future.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 drivers/char/random.c  | 17 ++++-------------
 include/linux/random.h |  2 +-
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 0158d3bff7e5..1251cb2cbab2 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
 EXPORT_SYMBOL(get_random_long);
 
 /*
- * randomize_range() returns a start address such that
- *
- *    [...... <range> .....]
- *  start                  end
- *
- * a <range> with size "len" starting at the return value is inside in the
- * area defined by [start, end], but is otherwise randomized.
+ * randomize_addr() returns a page aligned address within [start, start +
+ * range]
  */
 unsigned long
-randomize_range(unsigned long start, unsigned long end, unsigned long len)
+randomize_addr(unsigned long start, unsigned long range)
 {
-	unsigned long range = end - len - start;
-
-	if (end <= start + len)
-		return 0;
-	return PAGE_ALIGN(get_random_int() % range + start);
+	return PAGE_ALIGN(get_random_long() % range + start);
 }
 
 /* Interface for in-kernel drivers of true hardware RNGs.
diff --git a/include/linux/random.h b/include/linux/random.h
index e47e533742b5..1ad877a98186 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -34,7 +34,7 @@ extern const struct file_operations random_fops, urandom_fops;
 
 unsigned int get_random_int(void);
 unsigned long get_random_long(void);
-unsigned long randomize_range(unsigned long start, unsigned long end, unsigned long len);
+unsigned long randomize_addr(unsigned long start, unsigned long range);
 
 u32 prandom_u32(void);
 void prandom_bytes(void *buf, size_t nbytes);
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [RFC patch 2/6] x86: Use simpler API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  3:01     ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/x86/kernel/process.c    | 3 +--
 arch/x86/kernel/sys_x86_64.c | 5 +----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 96becbbb52e0..a083a2c0744e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -507,8 +507,7 @@ unsigned long arch_align_stack(unsigned long sp)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
 
 /*
diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index 10e0272d789a..f9cad22808fc 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -101,7 +101,6 @@ static void find_start_end(unsigned long flags, unsigned long *begin,
 			   unsigned long *end)
 {
 	if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT)) {
-		unsigned long new_begin;
 		/* This is usually used needed to map code in small
 		   model, so it needs to be in the first 31bit. Limit
 		   it to that.  This means we need to move the
@@ -112,9 +111,7 @@ static void find_start_end(unsigned long flags, unsigned long *begin,
 		*begin = 0x40000000;
 		*end = 0x80000000;
 		if (current->flags & PF_RANDOMIZE) {
-			new_begin = randomize_range(*begin, *begin + 0x02000000, 0);
-			if (new_begin)
-				*begin = new_begin;
+			*begin = randomize_addr(*begin, 0x02000000);
 		}
 	} else {
 		*begin = current->mm->mmap_legacy_base;
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] [RFC patch 2/6] x86: Use simpler API for random address requests
@ 2016-07-26  3:01     ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/x86/kernel/process.c    | 3 +--
 arch/x86/kernel/sys_x86_64.c | 5 +----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 96becbbb52e0..a083a2c0744e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -507,8 +507,7 @@ unsigned long arch_align_stack(unsigned long sp)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
 
 /*
diff --git a/arch/x86/kernel/sys_x86_64.c b/arch/x86/kernel/sys_x86_64.c
index 10e0272d789a..f9cad22808fc 100644
--- a/arch/x86/kernel/sys_x86_64.c
+++ b/arch/x86/kernel/sys_x86_64.c
@@ -101,7 +101,6 @@ static void find_start_end(unsigned long flags, unsigned long *begin,
 			   unsigned long *end)
 {
 	if (!test_thread_flag(TIF_ADDR32) && (flags & MAP_32BIT)) {
-		unsigned long new_begin;
 		/* This is usually used needed to map code in small
 		   model, so it needs to be in the first 31bit. Limit
 		   it to that.  This means we need to move the
@@ -112,9 +111,7 @@ static void find_start_end(unsigned long flags, unsigned long *begin,
 		*begin = 0x40000000;
 		*end = 0x80000000;
 		if (current->flags & PF_RANDOMIZE) {
-			new_begin = randomize_range(*begin, *begin + 0x02000000, 0);
-			if (new_begin)
-				*begin = new_begin;
+			*begin = randomize_addr(*begin, 0x02000000);
 		}
 	} else {
 		*begin = current->mm->mmap_legacy_base;
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [RFC patch 3/6] ARM: Use simpler API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  3:01     ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 4a803c5a1ff7..02dee671cded 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -314,8 +314,7 @@ unsigned long get_wchan(struct task_struct *p)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
 
 #ifdef CONFIG_MMU
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] [RFC patch 3/6] ARM: Use simpler API for random address requests
@ 2016-07-26  3:01     ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 4a803c5a1ff7..02dee671cded 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -314,8 +314,7 @@ unsigned long get_wchan(struct task_struct *p)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
 
 #ifdef CONFIG_MMU
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [RFC patch 4/6] arm64: Use simpler API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  3:01     ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm64/kernel/process.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 6cd2612236dc..11bf454baf86 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -374,12 +374,8 @@ unsigned long arch_align_stack(unsigned long sp)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk;
-
 	if (is_compat_task())
-		range_end += 0x02000000;
+		return randomize_addr(mm->brk, 0x02000000);
 	else
-		range_end += 0x40000000;
-
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+		return randomize_addr(mm->brk, 0x40000000);
 }
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] [RFC patch 4/6] arm64: Use simpler API for random address requests
@ 2016-07-26  3:01     ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/arm64/kernel/process.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 6cd2612236dc..11bf454baf86 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -374,12 +374,8 @@ unsigned long arch_align_stack(unsigned long sp)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk;
-
 	if (is_compat_task())
-		range_end += 0x02000000;
+		return randomize_addr(mm->brk, 0x02000000);
 	else
-		range_end += 0x40000000;
-
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+		return randomize_addr(mm->brk, 0x40000000);
 }
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [RFC patch 5/6] tile: Use simpler API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  3:01     ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/tile/mm/mmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/tile/mm/mmap.c b/arch/tile/mm/mmap.c
index 851a94e6ae58..50f6a693a2b6 100644
--- a/arch/tile/mm/mmap.c
+++ b/arch/tile/mm/mmap.c
@@ -88,6 +88,5 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] [RFC patch 5/6] tile: Use simpler API for random address requests
@ 2016-07-26  3:01     ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:01 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/tile/mm/mmap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/tile/mm/mmap.c b/arch/tile/mm/mmap.c
index 851a94e6ae58..50f6a693a2b6 100644
--- a/arch/tile/mm/mmap.c
+++ b/arch/tile/mm/mmap.c
@@ -88,6 +88,5 @@ void arch_pick_mmap_layout(struct mm_struct *mm)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [RFC patch 6/6] unicore32: Use simpler API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  3:02     ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:02 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/unicore32/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/unicore32/kernel/process.c b/arch/unicore32/kernel/process.c
index 00299c927852..b856178cf167 100644
--- a/arch/unicore32/kernel/process.c
+++ b/arch/unicore32/kernel/process.c
@@ -295,8 +295,7 @@ unsigned long get_wchan(struct task_struct *p)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
 
 /*
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] [RFC patch 6/6] unicore32: Use simpler API for random address requests
@ 2016-07-26  3:02     ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:02 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

Currently, all callers to randomize_range() set the length to 0 and
calculate end by adding a constant to the start address.  We can
simplify the API to remove a bunch of needless checks and variables.

Use the new randomize_addr(start, range) call to set the requested
address.

Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
 arch/unicore32/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/unicore32/kernel/process.c b/arch/unicore32/kernel/process.c
index 00299c927852..b856178cf167 100644
--- a/arch/unicore32/kernel/process.c
+++ b/arch/unicore32/kernel/process.c
@@ -295,8 +295,7 @@ unsigned long get_wchan(struct task_struct *p)
 
 unsigned long arch_randomize_brk(struct mm_struct *mm)
 {
-	unsigned long range_end = mm->brk + 0x02000000;
-	return randomize_range(mm->brk, range_end, 0) ? : mm->brk;
+	return randomize_addr(mm->brk, 0x02000000);
 }
 
 /*
-- 
2.9.2

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  3:30     ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:30 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: linux, akpm, keescook, tytso, arnd, gregkh, catalin.marinas,
	will.deacon, ralf, benh, paulus, mpe, davem, tglx, mingo, hpa,
	x86, viro, nnk, jeffv, alyzyn, dcashman

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
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26  3:30     ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26  3:30 UTC (permalink / raw)
  To: william.c.roberts, linux-mm, linux-kernel, kernel-hardening
  Cc: linux, akpm, keescook, tytso, arnd, gregkh, catalin.marinas,
	will.deacon, ralf, benh, paulus, mpe, davem, tglx, mingo, hpa,
	x86, viro, nnk, jeffv, alyzyn, dcashman

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
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26  3:30     ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  4:39       ` Kees Cook
  -1 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26  4:39 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> All,
>
> On Tue, Jul 26, 2016 at 03:01:55AM +0000, Jason Cooper wrote:
>> To date, all callers of randomize_range() have set the length to 0, and
>> check for a zero return value.  For the current callers, the only way
>> to get zero returned is if end <= start.  Since they are all adding a
>> constant to the start address, this is unnecessary.
>>
>> We can remove a bunch of needless checks by simplifying the API to do
>> just what everyone wants, return an address between [start, start +
>> range].
>>
>> While we're here, s/get_random_int/get_random_long/.  No current call
>> site is adversely affected by get_random_int(), since all current range
>> requests are < MAX_UINT.  However, we should match caller expectations
>> to avoid coming up short (ha!) in the future.
>>
>> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> ---
>>  drivers/char/random.c  | 17 ++++-------------
>>  include/linux/random.h |  2 +-
>>  2 files changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/char/random.c b/drivers/char/random.c
>> index 0158d3bff7e5..1251cb2cbab2 100644
>> --- a/drivers/char/random.c
>> +++ b/drivers/char/random.c
>> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>>  EXPORT_SYMBOL(get_random_long);
>>
>>  /*
>> - * randomize_range() returns a start address such that
>> - *
>> - *    [...... <range> .....]
>> - *  start                  end
>> - *
>> - * a <range> with size "len" starting at the return value is inside in the
>> - * area defined by [start, end], but is otherwise randomized.
>> + * randomize_addr() returns a page aligned address within [start, start +
>> + * range]
>>   */
>>  unsigned long
>> -randomize_range(unsigned long start, unsigned long end, unsigned long len)
>> +randomize_addr(unsigned long start, unsigned long range)
>>  {
>> -     unsigned long range = end - len - start;
>> -
>> -     if (end <= start + len)
>> -             return 0;
>> -     return PAGE_ALIGN(get_random_int() % range + start);
>> +     return PAGE_ALIGN(get_random_long() % range + start);
>>  }
>
> bah!  old patch file.  This should have been:
>
> if (range == 0)
>         return start;
> else
>         return PAGE_ALIGN(get_random_long() % range + start);

I think range should be limited to start + range < UINTMAX, and it
should be very clear if the range is inclusive or exclusive.  start =
0, range = 4096. does this mean 1 page, or 2 pages possible?

-Kees

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



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26  4:39       ` Kees Cook
  0 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26  4:39 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> All,
>
> On Tue, Jul 26, 2016 at 03:01:55AM +0000, Jason Cooper wrote:
>> To date, all callers of randomize_range() have set the length to 0, and
>> check for a zero return value.  For the current callers, the only way
>> to get zero returned is if end <= start.  Since they are all adding a
>> constant to the start address, this is unnecessary.
>>
>> We can remove a bunch of needless checks by simplifying the API to do
>> just what everyone wants, return an address between [start, start +
>> range].
>>
>> While we're here, s/get_random_int/get_random_long/.  No current call
>> site is adversely affected by get_random_int(), since all current range
>> requests are < MAX_UINT.  However, we should match caller expectations
>> to avoid coming up short (ha!) in the future.
>>
>> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> ---
>>  drivers/char/random.c  | 17 ++++-------------
>>  include/linux/random.h |  2 +-
>>  2 files changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/char/random.c b/drivers/char/random.c
>> index 0158d3bff7e5..1251cb2cbab2 100644
>> --- a/drivers/char/random.c
>> +++ b/drivers/char/random.c
>> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>>  EXPORT_SYMBOL(get_random_long);
>>
>>  /*
>> - * randomize_range() returns a start address such that
>> - *
>> - *    [...... <range> .....]
>> - *  start                  end
>> - *
>> - * a <range> with size "len" starting at the return value is inside in the
>> - * area defined by [start, end], but is otherwise randomized.
>> + * randomize_addr() returns a page aligned address within [start, start +
>> + * range]
>>   */
>>  unsigned long
>> -randomize_range(unsigned long start, unsigned long end, unsigned long len)
>> +randomize_addr(unsigned long start, unsigned long range)
>>  {
>> -     unsigned long range = end - len - start;
>> -
>> -     if (end <= start + len)
>> -             return 0;
>> -     return PAGE_ALIGN(get_random_int() % range + start);
>> +     return PAGE_ALIGN(get_random_long() % range + start);
>>  }
>
> bah!  old patch file.  This should have been:
>
> if (range == 0)
>         return start;
> else
>         return PAGE_ALIGN(get_random_long() % range + start);

I think range should be limited to start + range < UINTMAX, and it
should be very clear if the range is inclusive or exclusive.  start =
0, range = 4096. does this mean 1 page, or 2 pages possible?

-Kees

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



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
@ 2016-07-26  4:44     ` Kees Cook
  -1 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26  4:44 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, Mark Salyzyn,
	Daniel Cashman

On Mon, Jul 25, 2016 at 8:01 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> To date, all callers of randomize_range() have set the length to 0, and
> check for a zero return value.  For the current callers, the only way
> to get zero returned is if end <= start.  Since they are all adding a
> constant to the start address, this is unnecessary.
>
> We can remove a bunch of needless checks by simplifying the API to do
> just what everyone wants, return an address between [start, start +
> range].
>
> While we're here, s/get_random_int/get_random_long/.  No current call
> site is adversely affected by get_random_int(), since all current range
> requests are < MAX_UINT.  However, we should match caller expectations
> to avoid coming up short (ha!) in the future.
>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
>  drivers/char/random.c  | 17 ++++-------------
>  include/linux/random.h |  2 +-
>  2 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 0158d3bff7e5..1251cb2cbab2 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>  EXPORT_SYMBOL(get_random_long);
>
>  /*
> - * randomize_range() returns a start address such that
> - *
> - *    [...... <range> .....]
> - *  start                  end
> - *
> - * a <range> with size "len" starting at the return value is inside in the
> - * area defined by [start, end], but is otherwise randomized.
> + * randomize_addr() returns a page aligned address within [start, start +
> + * range]
>   */
>  unsigned long
> -randomize_range(unsigned long start, unsigned long end, unsigned long len)
> +randomize_addr(unsigned long start, unsigned long range)

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

-Kees

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



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26  4:44     ` Kees Cook
  0 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26  4:44 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, Mark Salyzyn,
	Daniel Cashman

On Mon, Jul 25, 2016 at 8:01 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> To date, all callers of randomize_range() have set the length to 0, and
> check for a zero return value.  For the current callers, the only way
> to get zero returned is if end <= start.  Since they are all adding a
> constant to the start address, this is unnecessary.
>
> We can remove a bunch of needless checks by simplifying the API to do
> just what everyone wants, return an address between [start, start +
> range].
>
> While we're here, s/get_random_int/get_random_long/.  No current call
> site is adversely affected by get_random_int(), since all current range
> requests are < MAX_UINT.  However, we should match caller expectations
> to avoid coming up short (ha!) in the future.
>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
>  drivers/char/random.c  | 17 ++++-------------
>  include/linux/random.h |  2 +-
>  2 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 0158d3bff7e5..1251cb2cbab2 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>  EXPORT_SYMBOL(get_random_long);
>
>  /*
> - * randomize_range() returns a start address such that
> - *
> - *    [...... <range> .....]
> - *  start                  end
> - *
> - * a <range> with size "len" starting at the return value is inside in the
> - * area defined by [start, end], but is otherwise randomized.
> + * randomize_addr() returns a page aligned address within [start, start +
> + * range]
>   */
>  unsigned long
> -randomize_range(unsigned long start, unsigned long end, unsigned long len)
> +randomize_addr(unsigned long start, unsigned long range)

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

-Kees

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



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26  4:44     ` [kernel-hardening] " Kees Cook
@ 2016-07-26 15:55       ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26 15:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, Mark Salyzyn,
	Daniel Cashman

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26 15:55       ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26 15:55 UTC (permalink / raw)
  To: Kees Cook
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, Mark Salyzyn,
	Daniel Cashman

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26 15:55       ` [kernel-hardening] " Jason Cooper
@ 2016-07-26 16:40         ` Kees Cook
  -1 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26 16:40 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, Mark Salyzyn,
	Daniel Cashman

On Tue, Jul 26, 2016 at 8:55 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> On Mon, Jul 25, 2016 at 09:44:27PM -0700, Kees Cook wrote:
>> On Mon, Jul 25, 2016 at 8:01 PM, Jason Cooper <jason@lakedaemon.net> wrote:
>> > To date, all callers of randomize_range() have set the length to 0, and
>> > check for a zero return value.  For the current callers, the only way
>> > to get zero returned is if end <= start.  Since they are all adding a
>> > constant to the start address, this is unnecessary.
>> >
>> > We can remove a bunch of needless checks by simplifying the API to do
>> > just what everyone wants, return an address between [start, start +
>> > range].
>> >
>> > While we're here, s/get_random_int/get_random_long/.  No current call
>> > site is adversely affected by get_random_int(), since all current range
>> > requests are < MAX_UINT.  However, we should match caller expectations
>> > to avoid coming up short (ha!) in the future.
>> >
>> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> > ---
>> >  drivers/char/random.c  | 17 ++++-------------
>> >  include/linux/random.h |  2 +-
>> >  2 files changed, 5 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/drivers/char/random.c b/drivers/char/random.c
>> > index 0158d3bff7e5..1251cb2cbab2 100644
>> > --- a/drivers/char/random.c
>> > +++ b/drivers/char/random.c
>> > @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>> >  EXPORT_SYMBOL(get_random_long);
>> >
>> >  /*
>> > - * randomize_range() returns a start address such that
>> > - *
>> > - *    [...... <range> .....]
>> > - *  start                  end
>> > - *
>> > - * a <range> with size "len" starting at the return value is inside in the
>> > - * area defined by [start, end], but is otherwise randomized.
>> > + * randomize_addr() returns a page aligned address within [start, start +
>> > + * range]
>> >   */
>> >  unsigned long
>> > -randomize_range(unsigned long start, unsigned long end, unsigned long len)
>> > +randomize_addr(unsigned long start, unsigned long range)
>>
>> Also, this series isn't bisectable since randomize_range gets removed
>> here before the callers are updated. Perhaps add a macro that calls
>> randomize_addr with a BUG_ON for len != 0? (And then remove it in the
>> last patch?)
>
> No, I was thinking just add randomize_addr() in the first patch, convert
> all the callers, then the last patch would remove randomize_range().
>
> That way the last patch can be a cleanup in a later merge window if
> needed.

That works too! :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26 16:40         ` Kees Cook
  0 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26 16:40 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, Mark Salyzyn,
	Daniel Cashman

On Tue, Jul 26, 2016 at 8:55 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> On Mon, Jul 25, 2016 at 09:44:27PM -0700, Kees Cook wrote:
>> On Mon, Jul 25, 2016 at 8:01 PM, Jason Cooper <jason@lakedaemon.net> wrote:
>> > To date, all callers of randomize_range() have set the length to 0, and
>> > check for a zero return value.  For the current callers, the only way
>> > to get zero returned is if end <= start.  Since they are all adding a
>> > constant to the start address, this is unnecessary.
>> >
>> > We can remove a bunch of needless checks by simplifying the API to do
>> > just what everyone wants, return an address between [start, start +
>> > range].
>> >
>> > While we're here, s/get_random_int/get_random_long/.  No current call
>> > site is adversely affected by get_random_int(), since all current range
>> > requests are < MAX_UINT.  However, we should match caller expectations
>> > to avoid coming up short (ha!) in the future.
>> >
>> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> > ---
>> >  drivers/char/random.c  | 17 ++++-------------
>> >  include/linux/random.h |  2 +-
>> >  2 files changed, 5 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/drivers/char/random.c b/drivers/char/random.c
>> > index 0158d3bff7e5..1251cb2cbab2 100644
>> > --- a/drivers/char/random.c
>> > +++ b/drivers/char/random.c
>> > @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>> >  EXPORT_SYMBOL(get_random_long);
>> >
>> >  /*
>> > - * randomize_range() returns a start address such that
>> > - *
>> > - *    [...... <range> .....]
>> > - *  start                  end
>> > - *
>> > - * a <range> with size "len" starting at the return value is inside in the
>> > - * area defined by [start, end], but is otherwise randomized.
>> > + * randomize_addr() returns a page aligned address within [start, start +
>> > + * range]
>> >   */
>> >  unsigned long
>> > -randomize_range(unsigned long start, unsigned long end, unsigned long len)
>> > +randomize_addr(unsigned long start, unsigned long range)
>>
>> Also, this series isn't bisectable since randomize_range gets removed
>> here before the callers are updated. Perhaps add a macro that calls
>> randomize_addr with a BUG_ON for len != 0? (And then remove it in the
>> last patch?)
>
> No, I was thinking just add randomize_addr() in the first patch, convert
> all the callers, then the last patch would remove randomize_range().
>
> That way the last patch can be a cleanup in a later merge window if
> needed.

That works too! :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26  4:39       ` [kernel-hardening] " Kees Cook
@ 2016-07-26 17:00         ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26 17:00 UTC (permalink / raw)
  To: Kees Cook
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26 17:00         ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-26 17:00 UTC (permalink / raw)
  To: Kees Cook
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26 17:00         ` [kernel-hardening] " Jason Cooper
@ 2016-07-26 17:07           ` Kees Cook
  -1 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26 17:07 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

On Tue, Jul 26, 2016 at 10:00 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> Hi Kees,
>
> On Mon, Jul 25, 2016 at 09:39:58PM -0700, Kees Cook wrote:
>> On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper <jason@lakedaemon.net> wrote:
>> > On Tue, Jul 26, 2016 at 03:01:55AM +0000, Jason Cooper wrote:
>> >> To date, all callers of randomize_range() have set the length to 0, and
>> >> check for a zero return value.  For the current callers, the only way
>> >> to get zero returned is if end <= start.  Since they are all adding a
>> >> constant to the start address, this is unnecessary.
>> >>
>> >> We can remove a bunch of needless checks by simplifying the API to do
>> >> just what everyone wants, return an address between [start, start +
>> >> range].
>> >>
>> >> While we're here, s/get_random_int/get_random_long/.  No current call
>> >> site is adversely affected by get_random_int(), since all current range
>> >> requests are < MAX_UINT.  However, we should match caller expectations
>
> merf. UINT_MAX.
>
>> >> to avoid coming up short (ha!) in the future.
>> >>
>> >> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> >> ---
>> >>  drivers/char/random.c  | 17 ++++-------------
>> >>  include/linux/random.h |  2 +-
>> >>  2 files changed, 5 insertions(+), 14 deletions(-)
>> >>
>> >> diff --git a/drivers/char/random.c b/drivers/char/random.c
>> >> index 0158d3bff7e5..1251cb2cbab2 100644
>> >> --- a/drivers/char/random.c
>> >> +++ b/drivers/char/random.c
>> >> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>> >>  EXPORT_SYMBOL(get_random_long);
>> >>
>> >>  /*
>> >> - * randomize_range() returns a start address such that
>> >> - *
>> >> - *    [...... <range> .....]
>> >> - *  start                  end
>> >> - *
>> >> - * a <range> with size "len" starting at the return value is inside in the
>> >> - * area defined by [start, end], but is otherwise randomized.
>> >> + * randomize_addr() returns a page aligned address within [start, start +
>> >> + * range]
>> >>   */
>> >>  unsigned long
>> >> -randomize_range(unsigned long start, unsigned long end, unsigned long len)
>> >> +randomize_addr(unsigned long start, unsigned long range)
>> >>  {
>> >> -     unsigned long range = end - len - start;
>> >> -
>> >> -     if (end <= start + len)
>> >> -             return 0;
>> >> -     return PAGE_ALIGN(get_random_int() % range + start);
>> >> +     return PAGE_ALIGN(get_random_long() % range + start);
>> >>  }
>> >
>> > bah!  old patch file.  This should have been:
>> >
>> > if (range == 0)
>> >         return start;
>> > else
>> >         return PAGE_ALIGN(get_random_long() % range + start);
>>
>> I think range should be limited to start + range < UINTMAX,
>
> ULONG_MAX?  I agree.

Heh, I am plagued by misspelling these constants, and yes, sorry, ULONG_MAX. :)

> if (range == 0 || ULONG_MAX - range < start)
>         return start;

Should it "abort" like this? I was thinking just cap the range, something like:

if (range > ULONG_MAX - start)
    range = ULONG_MAX - start

> else
>         return PAGE_ALIGN(get_random_long() % range + start);
>
> ?
>
>> and it should be very clear if the range is inclusive or exclusive.
>
> Sorry, I was reading the original comment, '[start, end]'  with square
> brackets denoting inclusive.
>
> Regardless, the math in randomize_range() was just undoing the math at
> each of the call sites.  This proposed change to randomize_addr()
> doesn't alter the current state of affairs wrt inclusive, exclusive.
>
>> start = 0, range = 4096. does this mean 1 page, or 2 pages possible?
>
> ooh, good spot.  What we have right now is [start, start + range), which
> is matching previous behavior.  But does not match the old comment,
> [start, end].  It should have been [start, end).
>
> So, you're correct, I need to clarify this in the comments.

Okay, cool. Thanks! I'm glad to have this clean-up. :)

-Kees

>
> thx,
>
> Jason.



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26 17:07           ` Kees Cook
  0 siblings, 0 replies; 34+ messages in thread
From: Kees Cook @ 2016-07-26 17:07 UTC (permalink / raw)
  To: Jason Cooper
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

On Tue, Jul 26, 2016 at 10:00 AM, Jason Cooper <jason@lakedaemon.net> wrote:
> Hi Kees,
>
> On Mon, Jul 25, 2016 at 09:39:58PM -0700, Kees Cook wrote:
>> On Mon, Jul 25, 2016 at 8:30 PM, Jason Cooper <jason@lakedaemon.net> wrote:
>> > On Tue, Jul 26, 2016 at 03:01:55AM +0000, Jason Cooper wrote:
>> >> To date, all callers of randomize_range() have set the length to 0, and
>> >> check for a zero return value.  For the current callers, the only way
>> >> to get zero returned is if end <= start.  Since they are all adding a
>> >> constant to the start address, this is unnecessary.
>> >>
>> >> We can remove a bunch of needless checks by simplifying the API to do
>> >> just what everyone wants, return an address between [start, start +
>> >> range].
>> >>
>> >> While we're here, s/get_random_int/get_random_long/.  No current call
>> >> site is adversely affected by get_random_int(), since all current range
>> >> requests are < MAX_UINT.  However, we should match caller expectations
>
> merf. UINT_MAX.
>
>> >> to avoid coming up short (ha!) in the future.
>> >>
>> >> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> >> ---
>> >>  drivers/char/random.c  | 17 ++++-------------
>> >>  include/linux/random.h |  2 +-
>> >>  2 files changed, 5 insertions(+), 14 deletions(-)
>> >>
>> >> diff --git a/drivers/char/random.c b/drivers/char/random.c
>> >> index 0158d3bff7e5..1251cb2cbab2 100644
>> >> --- a/drivers/char/random.c
>> >> +++ b/drivers/char/random.c
>> >> @@ -1822,22 +1822,13 @@ unsigned long get_random_long(void)
>> >>  EXPORT_SYMBOL(get_random_long);
>> >>
>> >>  /*
>> >> - * randomize_range() returns a start address such that
>> >> - *
>> >> - *    [...... <range> .....]
>> >> - *  start                  end
>> >> - *
>> >> - * a <range> with size "len" starting at the return value is inside in the
>> >> - * area defined by [start, end], but is otherwise randomized.
>> >> + * randomize_addr() returns a page aligned address within [start, start +
>> >> + * range]
>> >>   */
>> >>  unsigned long
>> >> -randomize_range(unsigned long start, unsigned long end, unsigned long len)
>> >> +randomize_addr(unsigned long start, unsigned long range)
>> >>  {
>> >> -     unsigned long range = end - len - start;
>> >> -
>> >> -     if (end <= start + len)
>> >> -             return 0;
>> >> -     return PAGE_ALIGN(get_random_int() % range + start);
>> >> +     return PAGE_ALIGN(get_random_long() % range + start);
>> >>  }
>> >
>> > bah!  old patch file.  This should have been:
>> >
>> > if (range == 0)
>> >         return start;
>> > else
>> >         return PAGE_ALIGN(get_random_long() % range + start);
>>
>> I think range should be limited to start + range < UINTMAX,
>
> ULONG_MAX?  I agree.

Heh, I am plagued by misspelling these constants, and yes, sorry, ULONG_MAX. :)

> if (range == 0 || ULONG_MAX - range < start)
>         return start;

Should it "abort" like this? I was thinking just cap the range, something like:

if (range > ULONG_MAX - start)
    range = ULONG_MAX - start

> else
>         return PAGE_ALIGN(get_random_long() % range + start);
>
> ?
>
>> and it should be very clear if the range is inclusive or exclusive.
>
> Sorry, I was reading the original comment, '[start, end]'  with square
> brackets denoting inclusive.
>
> Regardless, the math in randomize_range() was just undoing the math at
> each of the call sites.  This proposed change to randomize_addr()
> doesn't alter the current state of affairs wrt inclusive, exclusive.
>
>> start = 0, range = 4096. does this mean 1 page, or 2 pages possible?
>
> ooh, good spot.  What we have right now is [start, start + range), which
> is matching previous behavior.  But does not match the old comment,
> [start, end].  It should have been [start, end).
>
> So, you're correct, I need to clarify this in the comments.

Okay, cool. Thanks! I'm glad to have this clean-up. :)

-Kees

>
> thx,
>
> Jason.



-- 
Kees Cook
Chrome OS & Brillo Security

^ permalink raw reply	[flat|nested] 34+ messages in thread

* RE: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26  3:30     ` [kernel-hardening] " Jason Cooper
@ 2016-07-26 17:33       ` Roberts, William C
  -1 siblings, 0 replies; 34+ messages in thread
From: Roberts, William C @ 2016-07-26 17:33 UTC (permalink / raw)
  To: Jason Cooper, linux-mm, linux-kernel, kernel-hardening
  Cc: linux, akpm, keescook, tytso, arnd, gregkh, catalin.marinas,
	will.deacon, ralf, benh, paulus, mpe, davem, tglx, mingo, hpa,
	x86, viro, nnk, jeffv, alyzyn, dcashman



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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] RE: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-26 17:33       ` Roberts, William C
  0 siblings, 0 replies; 34+ messages in thread
From: Roberts, William C @ 2016-07-26 17:33 UTC (permalink / raw)
  To: Jason Cooper, linux-mm, linux-kernel, kernel-hardening
  Cc: linux, akpm, keescook, tytso, arnd, gregkh, catalin.marinas,
	will.deacon, ralf, benh, paulus, mpe, davem, tglx, mingo, hpa,
	x86, viro, nnk, jeffv, alyzyn, dcashman



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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [kernel-hardening] [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
                     ` (7 preceding siblings ...)
  (?)
@ 2016-07-27 13:51   ` Yann Droneaud
  -1 siblings, 0 replies; 34+ messages in thread
From: Yann Droneaud @ 2016-07-27 13:51 UTC (permalink / raw)
  To: kernel-hardening, william.c.roberts, linux-mm, linux-kernel
  Cc: Jason Cooper, linux, akpm, keescook, tytso, arnd, gregkh,
	catalin.marinas, will.deacon, ralf, benh, paulus, mpe, davem,
	tglx, mingo, hpa, x86, viro, nnk, jeffv, alyzyn, dcashman

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [RFC patch 1/6] random: Simplify API for random address requests
  2016-07-26 17:07           ` [kernel-hardening] " Kees Cook
@ 2016-07-28 19:02             ` Jason Cooper
  -1 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-28 19:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [kernel-hardening] Re: [RFC patch 1/6] random: Simplify API for random address requests
@ 2016-07-28 19:02             ` Jason Cooper
  0 siblings, 0 replies; 34+ messages in thread
From: Jason Cooper @ 2016-07-28 19:02 UTC (permalink / raw)
  To: Kees Cook
  Cc: Roberts, William C, linux-mm, LKML, kernel-hardening,
	Russell King - ARM Linux, Andrew Morton, Theodore Ts'o,
	Arnd Bergmann, Greg KH, Catalin Marinas, Will Deacon,
	Ralf Baechle, benh, Paul Mackerras, Michael Ellerman,
	David S. Miller, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, Al Viro, Nick Kralevich, Jeffrey Vander Stoep, alyzyn,
	Daniel Cashman

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.

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2016-07-28 19:02 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-25 18:25 [PATCH] randomize_range: use random long instead of int william.c.roberts
2016-07-25 18:54 ` Kees Cook
2016-07-26  2:18 ` Jason Cooper
2016-07-26  3:01 ` [RFC patch 1/6] random: Simplify API for random address requests Jason Cooper
2016-07-26  3:01   ` [kernel-hardening] " Jason Cooper
2016-07-26  3:01   ` [RFC patch 2/6] x86: Use simpler " Jason Cooper
2016-07-26  3:01     ` [kernel-hardening] " Jason Cooper
2016-07-26  3:01   ` [RFC patch 3/6] ARM: " Jason Cooper
2016-07-26  3:01     ` [kernel-hardening] " Jason Cooper
2016-07-26  3:01   ` [RFC patch 4/6] arm64: " Jason Cooper
2016-07-26  3:01     ` [kernel-hardening] " Jason Cooper
2016-07-26  3:01   ` [RFC patch 5/6] tile: " Jason Cooper
2016-07-26  3:01     ` [kernel-hardening] " Jason Cooper
2016-07-26  3:02   ` [RFC patch 6/6] unicore32: " Jason Cooper
2016-07-26  3:02     ` [kernel-hardening] " Jason Cooper
2016-07-26  3:30   ` [RFC patch 1/6] random: Simplify " Jason Cooper
2016-07-26  3:30     ` [kernel-hardening] " Jason Cooper
2016-07-26  4:39     ` Kees Cook
2016-07-26  4:39       ` [kernel-hardening] " Kees Cook
2016-07-26 17:00       ` Jason Cooper
2016-07-26 17:00         ` [kernel-hardening] " Jason Cooper
2016-07-26 17:07         ` Kees Cook
2016-07-26 17:07           ` [kernel-hardening] " Kees Cook
2016-07-28 19:02           ` Jason Cooper
2016-07-28 19:02             ` [kernel-hardening] " Jason Cooper
2016-07-26 17:33     ` Roberts, William C
2016-07-26 17:33       ` [kernel-hardening] " Roberts, William C
2016-07-26  4:44   ` Kees Cook
2016-07-26  4:44     ` [kernel-hardening] " Kees Cook
2016-07-26 15:55     ` Jason Cooper
2016-07-26 15:55       ` [kernel-hardening] " Jason Cooper
2016-07-26 16:40       ` Kees Cook
2016-07-26 16:40         ` [kernel-hardening] " Kees Cook
2016-07-27 13:51   ` [kernel-hardening] " Yann Droneaud

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.