Re: [PATCH] x86/boot/KASLR: Skip relocation handling in no kaslr case

[Baoquan He]

Sorry, forgot adding Cc: before maintainers contact, NACK this patch and
will repost.

On 06/24/17 at 10:16pm, Baoquan He 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")
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Baoquan He <bhe@xxxxxxxxxx>
> "H. Peter Anvin" <hpa@xxxxxxxxx>
> Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Ingo Molnar <mingo@xxxxxxxxxx>
> x86@xxxxxxxxxx
> Kees Cook <keescook@xxxxxxxxxxxx
> Baoquan He <bhe@xxxxxxxxxx>
> Dave Jiang <dave.jiang@xxxxxxxxx>
> Yinghai Lu <yinghai@xxxxxxxxxx>
> Arnd Bergmann <arnd@xxxxxxxx>
> Thomas Garnier <thgarnie@xxxxxxxxxx>
> ---
>  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;
>  
>  	/* 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
>