From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff Liu Subject: Re: [RESEND PATCH V3] binfmt_elf.c: use get_random_int() to fix entropy depleting Date: Wed, 07 Nov 2012 15:02:37 +0800 Message-ID: <509A078D.8060705@oracle.com> References: <5099F133.5030305@oracle.com> <5099FBAA.6000200@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: LKML , Andrew Morton , Andreas Dilger , John Sobecki , "viro@zeniv.linux.org.uk" , Alan Cox , "arnd@arndb.de" , James Morris , "Ted Ts'o" , "gregkh@linuxfoundation.org" , jakub@redhat.com, drepper@redhat.com, "linux-fsdevel@vger.kernel.org" To: Kees Cook Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On 11/07/2012 02:21 PM, Kees Cook wrote: > On Tue, Nov 6, 2012 at 10:11 PM, Jeff Liu wrote: >> Hello, >> >> This is the revised patch for fix entropy depleting. >> >> Changes: >> -------- >> v3->v2: >> - Tweak code comments of random_stack_user(). >> - Remove redundant bits mask and shift upon the random variable. >> >> v2->v1: >> Fix random copy to check up buffer length that are not 4-byte multiples. >> >> v2 can be found at: >> http://www.spinics.net/lists/linux-fsdevel/msg59418.html >> v1 can be found at: >> http://www.spinics.net/lists/linux-fsdevel/msg59128.html >> >> Many thanks to Andreas, Andrew as well as Kees for reviewing the patch of past! >> -Jeff >> >> >> Entropy is quickly depleted under normal 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 occurring since 2.6.30 with >> fs/binfmt_elf.c: create_elf_tables()->get_random_bytes(), introduced by >> f06295b44c296c8f ("ELF: implement AT_RANDOM for glibc PRNG seeding"). >> >> /* >> * Generate 16 random bytes for userspace PRNG seeding. >> */ >> get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes)); >> >> The patch introduces a wrapper around 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 >> >> Analyzed by John Sobecki. >> >> Signed-off-by: Jie Liu >> Cc: John Sobecki >> Cc: Al Viro >> Cc: Andreas Dilger >> Cc: Alan Cox >> Cc: Arnd Bergmann >> Cc: James Morris >> Cc: Ted Ts'o >> Cc: Greg Kroah-Hartman >> Cc: Kees Cook >> Cc: Jakub Jelinek >> Cc: Ulrich Drepper >> Signed-off-by: Andrew Morton >> >> --- >> fs/binfmt_elf.c | 22 +++++++++++++++++++++- >> 1 files changed, 21 insertions(+), 1 deletions(-) >> >> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c >> index fbd9f60..b6c59f6 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); > > I think it would be easier to just move the function ahead of its use > to avoid the predeclaration. Yes, it's better. > >> >> /* >> * 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,25 @@ static unsigned long randomize_stack_top(unsigned long stack_top) >> #endif >> } >> >> +/* >> + * Use get_random_int() to implement AT_RANDOM while avoiding depletion >> + * of the entropy pool. >> + */ >> +static void randomize_stack_user(unsigned char *buf, size_t nbytes) > > I think this name needs changing -- it has nothing to do with the > stack except that that's where it ends up in userspace. Perhaps > "get_atrandom_bytes"? I racked my brains but can not think out a better name than yours. :) > >> +{ >> + unsigned char *p = buf; >> + >> + while (nbytes) { >> + unsigned int random_variable; >> + size_t chunk = min(nbytes, sizeof(unsigned int)); >> + >> + random_variable = get_random_int(); > > I still want to hear at least from Ted about this changes -- we would > be potentially increasing the predictability of these bytes... We would not increasing that if this routine would be used for AT_RANDOM only(and if the array keeping aligned to 4 bytes). Otherwise, it would be, so let's waiting for further feedbacks. Thanks, -Jeff > >> + 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 > > -Kees >