Re: [PATCH] x86, kaslr: propagate base load address calculation

[Kees Cook <keescook@chromium.org>]

On Tue, Feb 10, 2015 at 5:17 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> Commit e2b32e678 ("x86, kaslr: randomize module base load address") makes
> the base address for module to be unconditionally randomized in case when
> CONFIG_RANDOMIZE_BASE is defined and "nokaslr" option isn't present on the
> commandline.
>
> This is not consistent with how choose_kernel_location() decides whether
> it will randomize kernel load base.
>
> Namely, CONFIG_HIBERNATION disables kASLR (unless "kaslr" option is
> explicitly specified on kernel commandline), which makes the state space
> larger than what module loader is looking at. IOW CONFIG_HIBERNATION &&
> CONFIG_RANDOMIZE_BASE is a valid config option, kASLR wouldn't be applied
> by default in that case, but module loader is not aware of that.
>
> Instead of fixing the logic in module.c, this patch takes more generic
> aproach, and exposes __KERNEL_OFFSET macro, which calculates the real
> offset that has been established by choose_kernel_location() during boot.
> This can be used later by other kernel code as well (such as, but not
> limited to, live patching).
>
> OOPS offset dumper and module loader are converted to that they make use
> of this macro as well.
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Ah, yes! This is a good clean up. Thanks! I do see, however, one
corner case remaining: kASLR randomized to 0 offset. This will force
module ASLR off, which I think is a mistake. Perhaps we need to export
the kaslr state as a separate item to be checked directly, instead of
using __KERNEL_OFFSET?

-Kees

> ---
>  arch/x86/include/asm/page_types.h |  4 ++++
>  arch/x86/kernel/module.c          | 10 +---------
>  arch/x86/kernel/setup.c           |  4 ++--
>  3 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h
> index f97fbe3..7f18eaf 100644
> --- a/arch/x86/include/asm/page_types.h
> +++ b/arch/x86/include/asm/page_types.h
> @@ -46,6 +46,10 @@
>
>  #ifndef __ASSEMBLY__
>
> +/* Return kASLR relocation offset */
> +extern char _text[];
> +#define __KERNEL_OFFSET ((unsigned long)&_text - __START_KERNEL)
> +
>  extern int devmem_is_allowed(unsigned long pagenr);
>
>  extern unsigned long max_low_pfn_mapped;
> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
> index e69f988..d236bd2 100644
> --- a/arch/x86/kernel/module.c
> +++ b/arch/x86/kernel/module.c
> @@ -46,21 +46,13 @@ do {                                                        \
>
>  #ifdef CONFIG_RANDOMIZE_BASE
>  static unsigned long module_load_offset;
> -static int randomize_modules = 1;
>
>  /* Mutex protects the module_load_offset. */
>  static DEFINE_MUTEX(module_kaslr_mutex);
>
> -static int __init parse_nokaslr(char *p)
> -{
> -       randomize_modules = 0;
> -       return 0;
> -}
> -early_param("nokaslr", parse_nokaslr);
> -
>  static unsigned long int get_module_load_offset(void)
>  {
> -       if (randomize_modules) {
> +       if (__KERNEL_OFFSET) {
>                 mutex_lock(&module_kaslr_mutex);
>                 /*
>                  * Calculate the module_load_offset the first time this
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index c4648ada..08124a1 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -833,8 +833,8 @@ dump_kernel_offset(struct notifier_block *self, unsigned long v, void *p)
>  {
>         pr_emerg("Kernel Offset: 0x%lx from 0x%lx "
>                  "(relocation range: 0x%lx-0x%lx)\n",
> -                (unsigned long)&_text - __START_KERNEL, __START_KERNEL,
> -                __START_KERNEL_map, MODULES_VADDR-1);
> +                __KERNEL_OFFSET, __START_KERNEL, __START_KERNEL_map,
> +                MODULES_VADDR-1);
>
>         return 0;
>  }
>
> --
> Jiri Kosina
> SUSE Labs



-- 
Kees Cook
Chrome OS Security