| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXu5j+1_j59LCWZ5RrjWpQHyAySPxMkiQTAAhuBqFmXc6wbgA@mail.gmail.com>
Date: Tue, 27 Jun 2017 15:42:24 -0700
From: Kees Cook <keescook@...omium.org>
To: Baoquan He <bhe@...hat.com>
Cc: LKML <linux-kernel@...r.kernel.org>,
"H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>,
"x86@...nel.org" <x86@...nel.org>,
"Kees Cook <keescook@...omium.org, Yinghai Lu" <yinghai@...nel.org>,
Arnd Bergmann <arnd@...db.de>,
Thomas Garnier <thgarnie@...gle.com>
Subject: Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case
On Sat, Jun 24, 2017 at 7:25 AM, Baoquan He <bhe@...hat.com> wrote:
> Kdump kernel will reset to firmware after crash is trigered when
> crashkernel=xxM,high is added to kernel command line. Kexec has the
> same phenomenon. This only happened on system with kaslr code
> compiled in and kernel option 'nokaslr'is added. Both of them works
> well when kaslr is enabled.
>
> When crashkernel high is set or kexec case, kexec/kdump kernel will be
> put above 4G. Since we assign the original loading address of kernel to
> virt_addr as initial value, the virt_addr will be larger than 1G if kaslr
> is disabled, it exceeds the kernel mapping size which is only 1G. Then
> it will cause relocation handling error in handle_relocations().
>
> In fact this is a known issue and fixed in commit:
>
> ... f285f4a ("x86, boot: Skip relocs when load address unchanged")
>
> But above fix was lost carelessly in later commit:
>
> ... 8391c73 ("x86/KASLR: Randomize virtual address separately")
Hrm, this changes more than just fixing this missing logic. How about
just simply:
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index b3c5a5f030ce..4496a05d1f8a 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -339,6 +339,7 @@ asmlinkage __visible void *extract_kernel(void
*rmode, memptr heap,
{
const unsigned long kernel_total_size = VO__end - VO__text;
unsigned long virt_addr = (unsigned long)output;
+ unsigned long virt_addr_orig = virt_addr;
/* Retain x86 boot parameters pointer passed from startup_32/64. */
boot_params = rmode;
@@ -405,7 +406,12 @@ asmlinkage __visible void *extract_kernel(void
*rmode, memptr heap,
__decompress(input_data, input_len, NULL, NULL, output, output_len,
NULL, error);
parse_elf(output);
- handle_relocations(output, output_len, virt_addr);
+ /*
+ * 32-bit always performs relocations. 64-bit relocations are only
+ * needed if kASLR has chosen a different load address.
+ */
+ if (!IS_ENABLED(CONFIG_X86_64) || virt_addr != virt_addr_orig)
+ handle_relocations(output, output_len, virt_addr);
debug_putstr("done.\nBooting the kernel.\n");
return output;
}
>
> To fix it, just assign LOAD_PHYSICAL_ADDR to virt_addr as initial value.
> If no kaslr is taken, it will skip the relocation handling and jump out.
>
> Fixes: 8391c73 ("x86/KASLR: Randomize virtual address separately")
> Tested-by: Dave Young <dyoung@...hat.com>
> Signed-off-by: Baoquan He <bhe@...hat.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: x86@...nel.org
> Cc: Kees Cook <keescook@...omium.org
> Cc: Yinghai Lu <yinghai@...nel.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Thomas Garnier <thgarnie@...gle.com>
> ---
> arch/x86/boot/compressed/kaslr.c | 3 ---
> arch/x86/boot/compressed/misc.c | 4 ++--
> arch/x86/boot/compressed/misc.h | 2 --
> 3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index fe318b4..91f27ab 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -625,9 +625,6 @@ void choose_random_location(unsigned long input,
> {
> unsigned long random_addr, min_addr;
>
> - /* By default, keep output position unchanged. */
> - *virt_addr = *output;
> -
> if (cmdline_find_option_bool("nokaslr")) {
> warn("KASLR disabled: 'nokaslr' on cmdline.");
> return;
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index b3c5a5f0..c945acd 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -338,7 +338,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> unsigned long output_len)
> {
> const unsigned long kernel_total_size = VO__end - VO__text;
> - unsigned long virt_addr = (unsigned long)output;
> + unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
I don't like this being hard-coded. The logic for the output address
is already handled in the .S files, and may not be LOAD_PHYSICAL_ADDR.
I'd prefer just adding back the simple test of virt_addr changing.
-Kees
>
> /* Retain x86 boot parameters pointer passed from startup_32/64. */
> boot_params = rmode;
> @@ -397,7 +397,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
> #ifndef CONFIG_RELOCATABLE
> if ((unsigned long)output != LOAD_PHYSICAL_ADDR)
> error("Destination address does not match LOAD_PHYSICAL_ADDR");
> - if ((unsigned long)output != virt_addr)
> + if (virt_addr != LOAD_PHYSICAL_ADDR)
> error("Destination virtual address changed when not relocatable");
> #endif
>
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index 1c8355e..766a521 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -81,8 +81,6 @@ static inline void choose_random_location(unsigned long input,
> unsigned long output_size,
> unsigned long *virt_addr)
> {
> - /* No change from existing output location. */
> - *virt_addr = *output;
> }
> #endif
>
> --
> 2.5.5
>
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists