* [PATCH] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting
@ 2012-10-25 7:53 Jeff Liu
2012-10-26 18:52 ` Andreas Dilger
2012-11-01 7:22 ` [PATCH V2] " Jeff Liu
0 siblings, 2 replies; 6+ messages in thread
From: Jeff Liu @ 2012-10-25 7:53 UTC (permalink / raw)
To: linux-fsdevel@vger.kernel.org
Cc: viro@zeniv.linux.org.uk, Ted Ts'o, James Morris, arnd, gregkh,
John Sobecki
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 <jeff.liu@oracle.com>
Analyzed-by: John Sobecki <john.sobecki@oracle.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Arnd Bergmann <arnn@arndb.de>
CC: James Morris <james.l.morris@oracle.com>
CC: Ted Ts'o <tytso@mit.edu>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
fs/binfmt_elf.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index fbd9f60..4fc92d5 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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 */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting
2012-10-25 7:53 [PATCH] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting Jeff Liu
@ 2012-10-26 18:52 ` Andreas Dilger
2012-10-27 5:00 ` Jeff Liu
2012-11-01 7:22 ` [PATCH V2] " Jeff Liu
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2012-10-26 18:52 UTC (permalink / raw)
To: jeff.liu@oracle.com
Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
Ted Ts'o, James Morris, arnd@arndb.de,
gregkh@linuxfoundation.org, John Sobecki
On 2012-10-25, at 1:53, Jeff Liu <jeff.liu@oracle.com> wrote:
> 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 <jeff.liu@oracle.com>
> Analyzed-by: John Sobecki <john.sobecki@oracle.com>
> CC: Al Viro <viro@zeniv.linux.org.uk>
> CC: Arnd Bergmann <arnn@arndb.de>
> CC: James Morris <james.l.morris@oracle.com>
> CC: Ted Ts'o <tytso@mit.edu>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> fs/binfmt_elf.c | 16 +++++++++++++++-
> 1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index fbd9f60..4fc92d5 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -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));
This is passing the parameter in units of 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));
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
> + }
> +}
> +
> static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
> {
> struct file *interpreter = NULL; /* to shut gcc up */
> --
> 1.7.4.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting
2012-10-26 18:52 ` Andreas Dilger
@ 2012-10-27 5:00 ` Jeff Liu
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Liu @ 2012-10-27 5:00 UTC (permalink / raw)
To: Andreas Dilger
Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk,
Ted Ts'o, James Morris, arnd@arndb.de,
gregkh@linuxfoundation.org, John Sobecki
Hi Andreas,
On 10/27/2012 02:52 AM, Andreas Dilger wrote:
> On 2012-10-25, at 1:53, Jeff Liu <jeff.liu@oracle.com> wrote:
>
>> 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 <jeff.liu@oracle.com>
>> Analyzed-by: John Sobecki <john.sobecki@oracle.com>
>> CC: Al Viro <viro@zeniv.linux.org.uk>
>> CC: Arnd Bergmann <arnn@arndb.de>
>> CC: James Morris <james.l.morris@oracle.com>
>> CC: Ted Ts'o <tytso@mit.edu>
>> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> ---
>> fs/binfmt_elf.c | 16 +++++++++++++++-
>> 1 files changed, 15 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>> index fbd9f60..4fc92d5 100644
>> --- a/fs/binfmt_elf.c
>> +++ b/fs/binfmt_elf.c
>> @@ -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));
> This is passing the parameter in units of 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));
> 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
>
> Cheers, Andreas
>
>> + }
>> +}
>> +
>> static int load_elf_binary(struct linux_binprm *bprm, struct pt_regs *regs)
>> {
>> struct file *interpreter = NULL; /* to shut gcc up */
>> --
>> 1.7.4.1
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V2] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting
2012-10-25 7:53 [PATCH] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting Jeff Liu
2012-10-26 18:52 ` Andreas Dilger
@ 2012-11-01 7:22 ` Jeff Liu
2012-11-07 0:09 ` Andrew Morton
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Liu @ 2012-11-01 7:22 UTC (permalink / raw)
To: linux-fsdevel@vger.kernel.org
Cc: Andreas Dilger, Andrew Morton, arnd@arndb.de,
viro@zeniv.linux.org.uk, Alan Cox, Ted Ts'o,
gregkh@linuxfoundation.org, James Morris, John Sobecki
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 <jeff.liu@oracle.com>
Analyzed-by: John Sobecki <john.sobecki@oracle.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Andreas Dilger <aedilger@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Alan Cox <alan@linux.intel.com>
CC: Arnd Bergmann <arnn@arndb.de>
CC: James Morris <james.l.morris@oracle.com>
CC: Ted Ts'o <tytso@mit.edu>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
fs/binfmt_elf.c | 26 +++++++++++++++++++++++++-
1 files changed, 25 insertions(+), 1 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index fbd9f60..2c8121f 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -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 */
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V2] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting
2012-11-01 7:22 ` [PATCH V2] " Jeff Liu
@ 2012-11-07 0:09 ` Andrew Morton
2012-11-07 3:01 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-11-07 0:09 UTC (permalink / raw)
To: Jeff Liu
Cc: linux-fsdevel@vger.kernel.org, Andreas Dilger, arnd@arndb.de,
viro@zeniv.linux.org.uk, Alan Cox, Ted Ts'o,
gregkh@linuxfoundation.org, James Morris, John Sobecki,
Jakub Jelinek, Ulrich Drepper
On Thu, 01 Nov 2012 15:22:51 +0800
Jeff Liu <jeff.liu@oracle.com> 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 ;)
> 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
>
> ...
>
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -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;
> + }
> +}
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?
>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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V2] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting
2012-11-07 0:09 ` Andrew Morton
@ 2012-11-07 3:01 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2012-11-07 3:01 UTC (permalink / raw)
To: Andrew Morton
Cc: Jeff Liu, linux-fsdevel@vger.kernel.org, Andreas Dilger,
arnd@arndb.de, viro@zeniv.linux.org.uk, Alan Cox, Ted Ts'o,
gregkh@linuxfoundation.org, James Morris, John Sobecki,
Jakub Jelinek, Ulrich Drepper
On Tue, Nov 6, 2012 at 4:09 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 01 Nov 2012 15:22:51 +0800
> Jeff Liu <jeff.liu@oracle.com> 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. :)
>> 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
>>
>> ...
>>
>> --- a/fs/binfmt_elf.c
>> +++ b/fs/binfmt_elf.c
>> @@ -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;
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();"
>> +
>> + 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.
> Or perhaps it wasn't and the stronger get_random_bytes() works better -
> I don't know?
>
> >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.
> 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
--
Kees Cook
Chrome OS Security
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-11-07 3:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-25 7:53 [PATCH] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting Jeff Liu
2012-10-26 18:52 ` Andreas Dilger
2012-10-27 5:00 ` Jeff Liu
2012-11-01 7:22 ` [PATCH V2] " Jeff Liu
2012-11-07 0:09 ` Andrew Morton
2012-11-07 3:01 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).