On Tue, Nov 6, 2012 at 4:09 PM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 01 Nov 2012 15:22:51 +0800
> Jeff Liu <jeff.liu@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
