* [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).