Re: [PATCH] x86, kaslr: randomize module base load address

[Kees Cook]

On Fri, Feb 21, 2014 at 12:36 PM, Andrew Morton
<akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 21 Feb 2014 12:21:10 -0800 Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
>> From: Andy Honig <ahonig@xxxxxxxxxx>
>>
>> Randomize the load address of modules in the kernel to make kASLR
>> effective for modules.  Modules can only be loaded within a particular
>> range of virtual address space.  This patch adds 10 bits of entropy to
>> the load address by adding 1-1024 * PAGE_SIZE to the beginning range
>> where modules are loaded.
>>
>> Example kASLR boot without this change, with a single module loaded:
>> ---[ Modules ]---
>> 0xffffffffc0000000-0xffffffffc0001000           4K     ro     GLB x  pte
>> 0xffffffffc0001000-0xffffffffc0002000           4K     ro     GLB NX pte
>> 0xffffffffc0002000-0xffffffffc0004000           8K     RW     GLB NX pte
>> 0xffffffffc0004000-0xffffffffc0200000        2032K                   pte
>> 0xffffffffc0200000-0xffffffffff000000        1006M                   pmd
>> ---[ End Modules ]---
>>
>> Example kASLR boot after this change, same module loaded:
>> ---[ Modules ]---
>> 0xffffffffc0000000-0xffffffffc0200000           2M                   pmd
>> 0xffffffffc0200000-0xffffffffc03bf000        1788K                   pte
>> 0xffffffffc03bf000-0xffffffffc03c0000           4K     ro     GLB x  pte
>> 0xffffffffc03c0000-0xffffffffc03c1000           4K     ro     GLB NX pte
>> 0xffffffffc03c1000-0xffffffffc03c3000           8K     RW     GLB NX pte
>> 0xffffffffc03c3000-0xffffffffc0400000         244K                   pte
>> 0xffffffffc0400000-0xffffffffff000000        1004M                   pmd
>> ---[ End Modules ]---
>>
>> ...
>>
>> --- a/arch/x86/kernel/module.c
>> +++ b/arch/x86/kernel/module.c
>> @@ -28,6 +28,7 @@
>>  #include <linux/mm.h>
>>  #include <linux/gfp.h>
>>  #include <linux/jump_label.h>
>> +#include <linux/random.h>
>>
>>  #include <asm/page.h>
>>  #include <asm/pgtable.h>
>> @@ -43,13 +44,49 @@ do {                                                      \
>>  } while (0)
>>  #endif
>>
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +static unsigned long module_load_offset;
>> +static int randomize_modules = 1;
>
> It's pretty common for people to later come back and say "hey I want to
> set the default in Kconfig".  Perhaps we should do that from day 1.

I've been slapped down for adding more config options in the past, and
I think it's unlikely that people using CONFIG_RANDOMIZE_BASE won't
want the modules base randomized too. I think this is a safe default,
but if you see it as a requirement, I can change it.

> This implies that parse_nokaslr() will need to be renamed and taught to
> handle 0->1 changing.
>
>> +static int __init parse_nokaslr(char *p)
>> +{
>> +     randomize_modules = 0;
>> +     return 0;
>> +}
>> +early_param("nokaslr", parse_nokaslr);
>
> Documentation/kernel-parameters.txt, please.   This isn't hard :(

"nokaslr" is already documented. Do you mean adding a note about
modules as well to the existing documentation?

>> +static unsigned long int get_module_load_offset(void)
>> +{
>> +     if (randomize_modules) {
>> +             mutex_lock(&module_mutex);
>> +             /*
>> +              * Calculate the module_load_offset the first time this
>> +              * code is called. Once calculated it stays the same until
>> +              * reboot.
>> +              */
>> +             if (module_load_offset == 0)
>> +                     module_load_offset =
>> +                             (get_random_int() % 1024 + 1) * PAGE_SIZE;
>> +             mutex_unlock(&module_mutex);
>> +     }
>> +     return module_load_offset;
>> +}
>
> This seems unnecessarily complex and inefficient.  We only set
> module_load_offset a single time, so why not do it that way?
> Mark it __init, run it during initcalls then throw it away.

I'd like to make sure this is running well after the pRNG is up and
running. I can run some tests to see how the entropy looks if this is
done during __init, though.

>
>> +#else
>> +static unsigned long int get_module_load_offset(void)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  void *module_alloc(unsigned long size)
>>  {
>>       if (PAGE_ALIGN(size) > MODULES_LEN)
>>               return NULL;
>> -     return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END,
>> -                             GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL_EXEC,
>> -                             NUMA_NO_NODE, __builtin_return_address(0));
>> +     return __vmalloc_node_range(size, 1,
>> +                                 MODULES_VADDR + get_module_load_offset(),
>> +                                 MODULES_END, GFP_KERNEL | __GFP_HIGHMEM,
>> +                                 PAGE_KERNEL_EXEC, NUMA_NO_NODE,
>> +                                 __builtin_return_address(0));
>>  }
>>
>>  #ifdef CONFIG_X86_32
>

Thanks for the review!

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/