From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Liu Subject: Re: [PATCH] binfmt_elf.c: Introduce a wrapper of get_random_int() to fix entropy depleting Date: Sat, 27 Oct 2012 13:00:48 +0800 Message-ID: <508B6A80.2020801@oracle.com> References: <5088EFDF.7040505@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: "linux-fsdevel@vger.kernel.org" , "viro@zeniv.linux.org.uk" , "Ted Ts'o" , James Morris , "arnd@arndb.de" , "gregkh@linuxfoundation.org" , John Sobecki To: Andreas Dilger Return-path: Received: from rcsinet15.oracle.com ([148.87.113.117]:37328 "EHLO rcsinet15.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750834Ab2J0FBM (ORCPT ); Sat, 27 Oct 2012 01:01:12 -0400 In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: Hi Andreas, On 10/27/2012 02:52 AM, Andreas Dilger wrote: > On 2012-10-25, at 1:53, Jeff Liu 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 >> Analyzed-by: John Sobecki >> CC: Al Viro >> CC: Arnd Bergmann >> CC: James Morris >> CC: Ted Ts'o >> CC: Greg Kroah-Hartman >> --- >> 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