binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting

PATCH Superseded Jeff Liu 4 participants 6 messages Oct 25, 2012 – Nov 7, 2012

Subsystems (2)

VFS exec and binfmt

lore

Share & tools

lore

Jeff Liu

author

2012-10-25 07:54 UTC #0 lore raw

PATCH 1/1 binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting

Hello,

We observed an issue regarding entropy quickly depleting under any normal I/O operations like ls(1), cat(1),etc... for instance:

$ date; cat /proc/sys/kernel/random/entropy_avail 
Thu Oct 25 15:24:37 CST 2012
3264
$ date; cat /proc/sys/kernel/random/entropy_avail 
Thu Oct 25 15:24:40 CST 2012
2791
$ date; cat /proc/sys/kernel/random/entropy_avail 
Thu Oct 25 15:24:42 CST 2012
2581
$ date; cat /proc/sys/kernel/random/entropy_avail 
Thu Oct 25 15:24:43 CST 2012
2122

According to John's analysis, it started to happen with 2.6.30 with:
fs/binfmt_elf.c: create_elf_tables()->get_random_bytes() was introduced:
/*
* Generate 16 random bytes for userspace PRNG seeding.
*/
get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));

Here is proposal patch to replace get_random_bytes() with a wrapper function get_random_int() which has low overhead to generate randoms, it looks stupid but works:

$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:32 CST 2012
2546
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:33 CST 2012
2558
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:34 CST 2012
2572
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:36 CST 2012
2614

Also, I have a question about whether stack randomization tunable parameter could be considered in this point or not, i.e,
If the user disabled the stack randomization via "kernel.randomize_va_space=0" or /proc/... Does it sounds make sense if just copying the k_rand_bytes[] back to user space with current uninitialized stack stuff rather than filling it with really strong random bytes, something like:
/*
* Generate 16 random bytes for userspace PRNG seeding if randomize is required.
*/
if (current->flags & PF_RANDOMIZE)

	get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));

Above fix also works although Glibc->elf_loader need a random bytes array for stack guarding, which means that the user want to take the risk by disabling stack randomize.

Any comments are appreciated!
-Jeff

Signed-off-by Jie Liu <[email protected]>

Analyzed-by John Sobecki <[email protected]>

CC Al Viro <[email protected]>

CC Arnd Bergmann <[email protected]>

CC James Morris <[email protected]>

CC Ted Ts'o <[email protected]>

CC Greg Kroah-Hartman <[email protected]>

---
 fs/binfmt_elf.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

fs/binfmt_elf.c +15 -1

index fbd9f60..4fc92d5 100644
@@ -48,6 +48,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs);
 static int load_elf_library(struct file *);
 static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
 				int, int, unsigned long);
+static void randomize_stack_user(unsigned char *random_bytes, size_t nr);
 
 /*
  * If we don't support core dumping, then supply a NULL so we

@@ -200,7 +201,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 	/*
 	 * Generate 16 random bytes for userspace PRNG seeding.
 	 */
-	get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
+	randomize_stack_user(k_rand_bytes, sizeof(k_rand_bytes));
 	u_rand_bytes = (elf_addr_t __user *)
 		       STACK_ALLOC(p, sizeof(k_rand_bytes));
 	if (__copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))

@@ -558,6 +559,19 @@ static unsigned long randomize_stack_top(unsigned long stack_top)
 #endif
 }
 
+static void randomize_stack_user(unsigned char *random_bytes, size_t nr)
+{
+	unsigned int random_variable;
+	size_t i;
+
+	for (i = 0; i < nr; i += sizeof(random_variable)) {
+		random_variable = get_random_int() & STACK_RND_MASK;
+		random_variable <= PAGE_SHIFT;
+		memcpy(&random_bytes[i], &random_variable,
+		       sizeof(random_variable));
+	}
+}
+
 static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */

Signature · 1 line

1.7.4.1

Andreas Dilger

2012-10-26 18:57 UTC #1 lore raw

Quoted 82 lines · Jeff Liu

We observed an issue regarding entropy quickly depleting under any normal I/O operations like ls(1), cat(1),etc... for instance:
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:37 CST 2012
3264
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:40 CST 2012
2791
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:42 CST 2012
2581
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:43 CST 2012
2122
According to John's analysis, it started to happen with 2.6.30 with:
fs/binfmt_elf.c: create_elf_tables()->get_random_bytes() was introduced:
/*
* Generate 16 random bytes for userspace PRNG seeding.
*/
get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
Here is proposal patch to replace get_random_bytes() with a wrapper function get_random_int() which has low overhead to generate randoms, it looks stupid but works:
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:32 CST 2012
2546
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:33 CST 2012
2558
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:34 CST 2012
2572
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:36 CST 2012
2614
Also, I have a question about whether stack randomization tunable parameter could be considered in this point or not, i.e,
If the user disabled the stack randomization via "kernel.randomize_va_space=0" or /proc/... Does it sounds make sense if just copying the k_rand_bytes[] back to user space with current uninitialized stack stuff rather than filling it with really strong random bytes, something like:
/*
* Generate 16 random bytes for userspace PRNG seeding if randomize is required.
*/
if (current->flags & PF_RANDOMIZE)
get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
Above fix also works although Glibc->elf_loader need a random bytes array for stack guarding, which means that the user want to take the risk by disabling stack randomize.
Any comments are appreciated!
-Jeff
Signed-off-by Jie Liu <[email protected]>
Analyzed-by John Sobecki <[email protected]>
CC Al Viro <[email protected]>
CC Arnd Bergmann <[email protected]>
CC James Morris <[email protected]>
CC Ted Ts'o <[email protected]>
CC Greg Kroah-Hartman <[email protected]>
---
fs/binfmt_elf.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
fs/binfmt_elf.c +1 -0
index fbd9f60..4fc92d5 100644
@@ -48,6 +48,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs);
 static int load_elf_library(struct file *);
 static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
               int, int, unsigned long);
+static void randomize_stack_user(unsigned char *random_bytes, size_t nr);
 
 /*
 * If we don't support core dumping, then supply a NULL so we

fs/binfmt_elf.c create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec, · line 204

Quoted 5 lines

fs/binfmt_elf.c +1 -1

@@ -200,7 +201,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
   /*
    * Generate 16 random bytes for userspace PRNG seeding.
    */
-    get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
+    randomize_stack_user(k_rand_bytes, sizeof(k_rand_bytes));

This is passing the parameter in units of bytes.

Quoted 7 lines

fs/binfmt_elf.c +0 -0

   u_rand_bytes = (elf_addr_t __user *)
              STACK_ALLOC(p, sizeof(k_rand_bytes));
   if (__copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))

@@ -558,6 +559,19 @@ static unsigned long randomize_stack_top(unsigned long stack_top)
 #endif
 }

fs/binfmt_elf.c static unsigned long randomize_stack_top(unsigned long stack_top) · line 571

Quoted 10 lines

fs/binfmt_elf.c +10 -0

+static void randomize_stack_user(unsigned char *random_bytes, size_t nr)
+{
+    unsigned int random_variable;
+    size_t i;
+
+    for (i = 0; i < nr; i += sizeof(random_variable)) {
+        random_variable = get_random_int() & STACK_RND_MASK;
+        random_variable <= PAGE_SHIFT;
+        memcpy(&random_bytes[i], &random_variable,
+               sizeof(random_variable));

This is filling in the the buffer in 4-byte increments. Are there any callers that will have buffers that are not 4-byte multiples? It would probably be safer to change the memcpy() to have a shorter length if the buffer is short.

Cheers, Andreas

Quoted 12 lines

fs/binfmt_elf.c +3 -0

+    }
+}
+
 static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 {
   struct file *interpreter = NULL; /* to shut gcc up */

Signature · 5 lines

1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Liu

author

2012-10-27 05:01 UTC #2 lore raw

Hi Andreas,

Quoted 98 lines · Andreas Dilger

Quoted 82 lines · Jeff Liu
We observed an issue regarding entropy quickly depleting under any normal I/O operations like ls(1), cat(1),etc... for instance:
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:37 CST 2012
3264
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:40 CST 2012
2791
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:42 CST 2012
2581
$ date; cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:24:43 CST 2012
2122
According to John's analysis, it started to happen with 2.6.30 with:
fs/binfmt_elf.c: create_elf_tables()->get_random_bytes() was introduced:
/*
* Generate 16 random bytes for userspace PRNG seeding.
*/
get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
Here is proposal patch to replace get_random_bytes() with a wrapper function get_random_int() which has low overhead to generate randoms, it looks stupid but works:
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:32 CST 2012
2546
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:33 CST 2012
2558
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:34 CST 2012
2572
$ date;cat /proc/sys/kernel/random/entropy_avail
Thu Oct 25 15:31:36 CST 2012
2614
Also, I have a question about whether stack randomization tunable parameter could be considered in this point or not, i.e,
If the user disabled the stack randomization via "kernel.randomize_va_space=0" or /proc/... Does it sounds make sense if just copying the k_rand_bytes[] back to user space with current uninitialized stack stuff rather than filling it with really strong random bytes, something like:
/*
* Generate 16 random bytes for userspace PRNG seeding if randomize is required.
*/
if (current->flags & PF_RANDOMIZE)
    get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
Above fix also works although Glibc->elf_loader need a random bytes array for stack guarding, which means that the user want to take the risk by disabling stack randomize.
Any comments are appreciated!
-Jeff
Signed-off-by Jie Liu <[email protected]>
Analyzed-by John Sobecki <[email protected]>
CC Al Viro <[email protected]>
CC Arnd Bergmann <[email protected]>
CC James Morris <[email protected]>
CC Ted Ts'o <[email protected]>
CC Greg Kroah-Hartman <[email protected]>
---
fs/binfmt_elf.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
fs/binfmt_elf.c +1 -0
index fbd9f60..4fc92d5 100644
@@ -48,6 +48,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs);
 static int load_elf_library(struct file *);
 static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
                int, int, unsigned long);
+static void randomize_stack_user(unsigned char *random_bytes, size_t nr);
 
 /*
  * If we don't support core dumping, then supply a NULL so we
fs/binfmt_elf.c create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec, · line 204
Quoted 5 lines
fs/binfmt_elf.c +1 -1
@@ -200,7 +201,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
    /*
     * Generate 16 random bytes for userspace PRNG seeding.
     */
-    get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
+    randomize_stack_user(k_rand_bytes, sizeof(k_rand_bytes));
This is passing the parameter in units of bytes.
Quoted 7 lines
fs/binfmt_elf.c +0 -0
    u_rand_bytes = (elf_addr_t __user *)
               STACK_ALLOC(p, sizeof(k_rand_bytes));
    if (__copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))
@@ -558,6 +559,19 @@ static unsigned long randomize_stack_top(unsigned long stack_top)
 #endif
 }
 

Quoted 11 lines

fs/binfmt_elf.c static unsigned long randomize_stack_top(unsigned long stack_top) · line 571
Quoted 10 lines
fs/binfmt_elf.c +10 -0
+static void randomize_stack_user(unsigned char *random_bytes, size_t nr)
+{
+    unsigned int random_variable;
+    size_t i;
+
+    for (i = 0; i < nr; i += sizeof(random_variable)) {
+        random_variable = get_random_int() & STACK_RND_MASK;
+        random_variable <= PAGE_SHIFT;
+        memcpy(&random_bytes[i], &random_variable,
+               sizeof(random_variable));
This is filling in the the buffer in 4-byte increments. Are there any callers that will have buffers that are not 4-byte multiples? It would probably be safer to change the memcpy() to have a shorter length if the buffer is short.

Thanks for your comments!

I also thought to make randomize_stack_user() safer as your mentioned at first, but I don't find other callers related to it in binfmt_elf.c. Anyway, the current logic is not generic to deal with various array length, I'll fix it.

Also, I even wonder if we can add above loops inside the create_elf_table() directly if there has no other callers?

Thanks,
-Jeff

Quoted 15 lines

Cheers, Andreas

Quoted 12 lines

fs/binfmt_elf.c +3 -0

+    }
+}
+
 static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 {
    struct file *interpreter = NULL; /* to shut gcc up */

Signature · 5 lines

1.7.4.1
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jeff Liu

author

2012-11-01 07:23 UTC #3 lore raw

PATCH v2 1/1 binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting

In reply to Jeff Liu

Hello,

Entropy quickly depleting under normal I/O operations like ls(1), cat(1), etc... between 2.6.30 to current mainline, for instance:

$ cat /proc/sys/kernel/random/entropy_avail 
3428
$ cat /proc/sys/kernel/random/entropy_avail 
2911
$cat /proc/sys/kernel/random/entropy_avail 
2620

We observed this problem has been occurred with fs/binfmt_elf.c: create_elf_tables()->get_random_bytes() was introduced began at 2.6.30.
/*
* Generate 16 random bytes for userspace PRNG seeding.
*/
get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));

This proposal patch is trying to introduce a wrapper of get_random_int() which has lower overhead than calling get_random_bytes() directly.

With this patch applied:

$ cat /proc/sys/kernel/random/entropy_avail
2731
$ cat /proc/sys/kernel/random/entropy_avail
2802
$ cat /proc/sys/kernel/random/entropy_avail
2878

v2->v1:
-------
- Fix random copy to check up buffer length that are not 4-byte multiples according to Andreas's comments, thank you.

v1 can be found at:
http://www.spinics.net/lists/linux-fsdevel/msg59128.html

Any comments are more than welcome!
-Jeff

Signed-off-by Jie Liu <[email protected]>

Analyzed-by John Sobecki <[email protected]>

CC Al Viro <[email protected]>

CC Andreas Dilger <[email protected]>

CC Andrew Morton <[email protected]>

CC Alan Cox <[email protected]>

CC Arnd Bergmann <[email protected]>

CC James Morris <[email protected]>

CC Ted Ts'o <[email protected]>

CC Greg Kroah-Hartman <[email protected]>

---
 fs/binfmt_elf.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

fs/binfmt_elf.c +25 -1

index fbd9f60..2c8121f 100644
@@ -48,6 +48,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs);
 static int load_elf_library(struct file *);
 static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
 				int, int, unsigned long);
+static void randomize_stack_user(unsigned char *buf, size_t nbytes);
 
 /*
  * If we don't support core dumping, then supply a NULL so we

@@ -200,7 +201,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 	/*
 	 * Generate 16 random bytes for userspace PRNG seeding.
 	 */
-	get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
+	randomize_stack_user(k_rand_bytes, sizeof(k_rand_bytes));
 	u_rand_bytes = (elf_addr_t __user *)
 		       STACK_ALLOC(p, sizeof(k_rand_bytes));
 	if (__copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))

@@ -558,6 +559,29 @@ static unsigned long randomize_stack_top(unsigned long stack_top)
 #endif
 }
 
+/*
+ * A wrapper of get_random_int() to generate random bytes which has lower
+ * overhead than call get_random_bytes() directly.
+ * create_elf_tables() call this function to generate 16 random bytes for
+ * userspace PRNG seeding.
+ */
+static void randomize_stack_user(unsigned char *buf, size_t nbytes)
+{
+	unsigned char *p = buf;
+
+	while (nbytes) {
+		unsigned int random_variable;
+		size_t chunk = min(nbytes, sizeof(unsigned int));
+
+		random_variable = get_random_int() & STACK_RND_MASK;
+		random_variable <<= PAGE_SHIFT;
+
+		memcpy(p, &random_variable, chunk);
+		p += chunk;
+		nbytes -= chunk;
+	}
+}
+
 static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 {
 	struct file *interpreter = NULL; /* to shut gcc up */

Signature · 1 line

1.7.4.1

Andrew Morton

2012-11-07 00:09 UTC #4 lore raw

Quoted 4 lines · On Thu, 01 Nov 2012 15:22:51 +0800 Jeff Liu <[email protected]> wrote:

Hello,
Entropy quickly depleting under normal I/O operations like ls(1), cat(1), etc... between 2.6.30 to current mainline,

Well that's bad. Let's cc Kees, who broke it ;)

Quoted 64 lines

for instance:

$ cat /proc/sys/kernel/random/entropy_avail 
3428
$ cat /proc/sys/kernel/random/entropy_avail 
2911
$cat /proc/sys/kernel/random/entropy_avail 
2620

This proposal patch is trying to introduce a wrapper of get_random_int() which has lower overhead than calling get_random_bytes() directly.

With this patch applied:

$ cat /proc/sys/kernel/random/entropy_avail
2731
$ cat /proc/sys/kernel/random/entropy_avail
2802
$ cat /proc/sys/kernel/random/entropy_avail
2878

...

fs/binfmt_elf.c +14 -1

@@ -48,6 +48,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs);
 static int load_elf_library(struct file *);
 static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
 				int, int, unsigned long);
+static void randomize_stack_user(unsigned char *buf, size_t nbytes);
 
 /*
  * If we don't support core dumping, then supply a NULL so we

@@ -200,7 +201,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
 	/*
 	 * Generate 16 random bytes for userspace PRNG seeding.
 	 */
-	get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
+	randomize_stack_user(k_rand_bytes, sizeof(k_rand_bytes));
 	u_rand_bytes = (elf_addr_t __user *)
 		       STACK_ALLOC(p, sizeof(k_rand_bytes));
 	if (__copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))

@@ -558,6 +559,29 @@ static unsigned long randomize_stack_top(unsigned long stack_top)
 #endif
 }
 
+/*
+ * A wrapper of get_random_int() to generate random bytes which has lower
+ * overhead than call get_random_bytes() directly.
+ * create_elf_tables() call this function to generate 16 random bytes for
+ * userspace PRNG seeding.
+ */
+static void randomize_stack_user(unsigned char *buf, size_t nbytes)
+{
+	unsigned char *p = buf;
+
+	while (nbytes) {
+		unsigned int random_variable;

fs/binfmt_elf.c static unsigned long randomize_stack_top(unsigned long stack_top) · line 583

Quoted 10 lines

fs/binfmt_elf.c +10 -0

+		size_t chunk = min(nbytes, sizeof(unsigned int));
+
+		random_variable = get_random_int() & STACK_RND_MASK;
+		random_variable <<= PAGE_SHIFT;
+
+		memcpy(p, &random_variable, chunk);
+		p += chunk;
+		nbytes -= chunk;
+	}
+}

Prior to f06295b44c296 ("ELF: implement AT_RANDOM for glibc PRNG seeding"), glibc was opening and using /dev/urandom for this. So presumably the urandom level of security was sufficient.

Or perhaps it wasn't and the stronger get_random_bytes() works better - I don't know?

Quoted 1 line

From my reading of the source, get_random_int() is weaker even than

/dev/urandom?

So my bottom line is: I don't know! Kees? Ted? Ulrich?

Kees Cook

2012-11-07 03:01 UTC #5 lore raw

Quoted 9 lines · Andrew Morton

Quoted 4 lines · On Thu, 01 Nov 2012 15:22:51 +0800 Jeff Liu <[email protected]> wrote:
Hello,
Entropy quickly depleting under normal I/O operations like ls(1), cat(1), etc... between 2.6.30 to current mainline,
Well that's bad. Let's cc Kees, who broke it ;)

There was a userspace need for this entropy, so I still think I _fixed_ things. :)

Quoted 52 lines

for instance:

$ cat /proc/sys/kernel/random/entropy_avail
3428
$ cat /proc/sys/kernel/random/entropy_avail
2911
$cat /proc/sys/kernel/random/entropy_avail
2620

This proposal patch is trying to introduce a wrapper of get_random_int() which has lower overhead than calling get_random_bytes() directly.

With this patch applied:

$ cat /proc/sys/kernel/random/entropy_avail
2731
$ cat /proc/sys/kernel/random/entropy_avail
2802
$ cat /proc/sys/kernel/random/entropy_avail
2878

...

fs/binfmt_elf.c +2 -1

@@ -48,6 +48,7 @@ static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs);
 static int load_elf_library(struct file *);
 static unsigned long elf_map(struct file *, unsigned long, struct elf_phdr *,
                              int, int, unsigned long);
+static void randomize_stack_user(unsigned char *buf, size_t nbytes);
 
 /*
  * If we don't support core dumping, then supply a NULL so we

@@ -200,7 +201,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
      /*
       * Generate 16 random bytes for userspace PRNG seeding.
       */
-     get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes));
+     randomize_stack_user(k_rand_bytes, sizeof(k_rand_bytes));
      u_rand_bytes = (elf_addr_t __user *)
                     STACK_ALLOC(p, sizeof(k_rand_bytes));
      if (__copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes)))

@@ -558,6 +559,29 @@ static unsigned long randomize_stack_top(unsigned long stack_top)
 #endif
 }

Quoted 16 lines

Quoted 6 lines

fs/binfmt_elf.c +6 -0

+/*
+ * A wrapper of get_random_int() to generate random bytes which has lower
+ * overhead than call get_random_bytes() directly.
+ * create_elf_tables() call this function to generate 16 random bytes for
+ * userspace PRNG seeding.
+ */

fs/binfmt_elf.c static unsigned long randomize_stack_top(unsigned long stack_top) · line 577

Quoted 10 lines

fs/binfmt_elf.c +10 -0

+static void randomize_stack_user(unsigned char *buf, size_t nbytes)
+{
+     unsigned char *p = buf;
+
+     while (nbytes) {
+             unsigned int random_variable;
+             size_t chunk = min(nbytes, sizeof(unsigned int));
+
+             random_variable = get_random_int() & STACK_RND_MASK;
+             random_variable <<= PAGE_SHIFT;

As I mentioned in the other email, this doesn't make sense to me. This logic was copied from the stack location randomization. I think the above two lines should just be "random_variable = get_random_int();"

Quoted 10 lines

fs/binfmt_elf.c static unsigned long randomize_stack_top(unsigned long stack_top) · line 583
Quoted 6 lines
fs/binfmt_elf.c +6 -0
+
+             memcpy(p, &random_variable, chunk);
+             p += chunk;
+             nbytes -= chunk;
+     }
+}
Prior to f06295b44c296 ("ELF: implement AT_RANDOM for glibc PRNG seeding"), glibc was opening and using /dev/urandom for this. So presumably the urandom level of security was sufficient.

Right, and I'm fine with that, and was part of the reasoning of the implementation. The "trouble" is that reading urandom does, in fact, deplete entropy. The main difference before/after AT_RANDOM is that not every distro built glibc with /dev/urandom support, even those the features that needed it were always in use (i.e. without this, glibc's stack protector and pointer mangling values were very predictable). It was felt it was better to push this all the way to the kernel so that all systems would have the same level of protection.

Quoted 5 lines

Or perhaps it wasn't and the stronger get_random_bytes() works better - I don't know?
Quoted 1 line
From my reading of the source, get_random_int() is weaker even than
/dev/urandom?

That's my understanding as well. I'm happy to defer to someone else that understand the randomness subsystem better than I do, but I'm loathe to increase the potential predictability of the value of AT_RANDOM.

Quoted 1 line

So my bottom line is: I don't know! Kees? Ted? Ulrich?

We're using get_random_int() for stack locations and, IIRC, some network packet things. If this is considered strong enough for those things, maybe it's not unreasonable to use it here too. So, it really just boils down to "Is get_random_int() really strong enough for these things?"

-Kees

Signature · 2 lines

Kees Cook
Chrome OS Security