Re: [PATCH 1/4] kaslr: check user's config too when handle relocations

Kees Cook
2014-09-05T17:11:13Z

On Fri, Sep 5, 2014 at 7:08 AM, Baoquan He <bhe@redhat.com> wrote:
> kaslr's action is splitted into 2 parts. The 1st is getting available memory
> slots and randomly choose the kernel relocation address. After decompression
> of kernel to the chosen place, the 2nd part begin to check if kaslr has got
> a relocation address, and will do the relocations handling if yes.
>
> However in current implementation, in the 2nd part, it doesn't check user's
> config, just compare decompression output address and the LOAD_PHYSICAL_ADDR
> where kernel was compiled to run. If they are equal, it means a kaslr is
> taking action, and need do the relocation handling. This truly works when
> bootloader always load kernel to LOAD_PHYSICAL_ADDR. But this doesn't always
> happens. Kexec/kdump kernel loading is exceptional. Kdump/kexec can load
> kernel anywhere, this is not fixed. So in this case, it will do the relocation
> handling though user clearly set nokaslr in cmdline. This is not correct.
>
> So in this patch, check user's config too in the 2nd part, namely in
> handle_relocations().
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
> arch/x86/boot/compressed/aslr.c | 2 ++
> arch/x86/boot/compressed/misc.c | 12 ++++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/arch/x86/boot/compressed/aslr.c b/arch/x86/boot/compressed/aslr.c
> index fc6091a..975b07b 100644
> --- a/arch/x86/boot/compressed/aslr.c
> +++ b/arch/x86/boot/compressed/aslr.c
> @@ -292,11 +292,13 @@ unsigned char *choose_kernel_location(unsigned char *input,
> #ifdef CONFIG_HIBERNATION
> if (!cmdline_find_option_bool("kaslr")) {
> debug_putstr("KASLR disabled by default...\n");
> + debug_putstr("No need to choose kernel relocation...\n");
> goto out;
> }
> #else
> if (cmdline_find_option_bool("nokaslr")) {
> debug_putstr("KASLR disabled by cmdline...\n");
> + debug_putstr("No need to choose kernel relocation...\n");
> goto out;
> }
> #endif

NAK on this part -- the whole point is to report the results of the
expected KASLR state based on build config, cmdline, etc.

> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index 57ab74d..7780a5b 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -238,6 +238,18 @@ static void handle_relocations(void *output, unsigned long output_len)
> unsigned long min_addr = (unsigned long)output;
> unsigned long max_addr = min_addr + output_len;
>
> +#ifdef CONFIG_HIBERNATION
> + if (!cmdline_find_option_bool("kaslr")) {
> + debug_putstr("No relocation needed... ");
> + return;
> + }
> +#else
> + if (cmdline_find_option_bool("nokaslr")) {
> + debug_putstr("No relocation needed... ");
> + return;
> + }
> +#endif
> +

I don't think this is correct. If you look at a02150610776 ("x86,
relocs: Move ELF relocation handling to C"), we always did relocations
on 32-bit when CONFIG_RELOCATABLE was set, so I think this will fail
badly on 32-bit. 64-bit only needs relocation when
CONFIG_RANDOMIZE_BASE is set, so this is probably what needs to be
tested here instead. I think a better option would be, in
decompress_kernel(), to compare output before and after
choose_kernel_location(). If it's the same on 64-bit,
handle_relocations() can be skipped. (Perhaps pass the before/after to
handle_relocations() and it can perform the logic.)

-Kees

> /*
> * Calculate the delta between where vmlinux was linked to load
> * and where it was actually loaded.
> --
> 1.8.5.3
>

--
Kees Cook
Chrome OS Security