LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
@ 2018-08-08 14:03 Mike Galbraith
  2018-08-09  4:21 ` Dave Young
  2018-08-10  8:45 ` Dave Young
  0 siblings, 2 replies; 11+ messages in thread
From: Mike Galbraith @ 2018-08-08 14:03 UTC (permalink / raw)
  To: Dave Young; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml

When booting with efi=noruntime, we call efi_runtime_map_copy() while
loading the kdump kernel, and trip over a NULL efi.memmap.map.  Avoid
that and a useless allocation when the only mapping we can use (1:1)
is not available.

Signed-off-by: Mike Galbraith <efault@gmx.de>
---
 arch/x86/kernel/kexec-bzimage64.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -122,9 +122,6 @@ static int setup_efi_info_memmap(struct
 	unsigned long efi_map_phys_addr = params_load_addr + efi_map_offset;
 	struct efi_info *ei = &params->efi_info;
 
-	if (!efi_map_sz)
-		return 0;
-
 	efi_runtime_map_copy(efi_map, efi_map_sz);
 
 	ei->efi_memmap = efi_map_phys_addr & 0xffffffff;
@@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
 	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
 	 * without efi.
 	 */
-	if (efi_enabled(EFI_OLD_MEMMAP))
+	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_MEMMAP))
 		return 0;
 
 	ei->efi_loader_signature = current_ei->efi_loader_signature;
@@ -338,7 +335,7 @@ static void *bzImage64_load(struct kimag
 	struct kexec_entry64_regs regs64;
 	void *stack;
 	unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
-	unsigned int efi_map_offset, efi_map_sz, efi_setup_data_offset;
+	unsigned int efi_map_offset = 0, efi_map_sz = 0, efi_setup_data_offset = 0;
 	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
 				  .top_down = true };
 	struct kexec_buf pbuf = { .image = image, .buf_min = MIN_PURGATORY_ADDR,
@@ -397,19 +394,22 @@ static void *bzImage64_load(struct kimag
 	 * have to create separate segment for each. Keeps things
 	 * little bit simple
 	 */
-	efi_map_sz = efi_get_runtime_map_size();
 	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
 				MAX_ELFCOREHDR_STR_LEN;
 	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
-	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16) +
-				sizeof(struct setup_data) +
-				sizeof(struct efi_setup_data);
+	kbuf.bufsz = params_cmdline_sz + sizeof(struct setup_data);
+
+	/* Now add space for the efi stuff if we have a useable 1:1 mapping. */
+	if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_MEMMAP)) {
+		efi_map_sz = efi_get_runtime_map_size();
+		kbuf.bufsz += ALIGN(efi_map_sz, 16) + sizeof(struct efi_setup_data);
+		efi_map_offset = params_cmdline_sz;
+		efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
+	}
 
 	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
 	if (!params)
 		return ERR_PTR(-ENOMEM);
-	efi_map_offset = params_cmdline_sz;
-	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
 
 	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
 	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-08 14:03 [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference Mike Galbraith
@ 2018-08-09  4:21 ` Dave Young
  2018-08-09  5:05   ` Mike Galbraith
  2018-08-09  7:33   ` Mike Galbraith
  2018-08-10  8:45 ` Dave Young
  1 sibling, 2 replies; 11+ messages in thread
From: Dave Young @ 2018-08-09  4:21 UTC (permalink / raw)
  To: Mike Galbraith; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml, kexec

Hi Mike,

Thanks for the patch!
On 08/08/18 at 04:03pm, Mike Galbraith wrote:
> When booting with efi=noruntime, we call efi_runtime_map_copy() while
> loading the kdump kernel, and trip over a NULL efi.memmap.map.  Avoid
> that and a useless allocation when the only mapping we can use (1:1)
> is not available.

At first glance, efi_get_runtime_map_size should return 0 in case
noruntime.

Also since we are here, would you mind to restructure the bzImage64_load
function, and try to move all efi related code to setup_efi_state()?


setup_boot_parameters(struct kimage *image, struct boot_params *params,
                      unsigned long params_load_addr,
                      unsigned int efi_map_offset, unsigned int efi_map_sz,
                      unsigned int efi_setup_data_offset)
{
[snip]

#ifdef CONFIG_EFI
        /* Setup EFI state */
        setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
                        efi_setup_data_offset);
#endif

[snip]
}

Currently bzImage64_load prepares the efi_map_offset, efi_map_sz,
 and efi_setup_data_offset and then pass it to setup_boot_parameters and
setup_efi_state.  It should be better to move those efi_* variables to
setup_efi_state().

So we can call setup_efi_state only when efi runtime is enabled.

> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> ---
>  arch/x86/kernel/kexec-bzimage64.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -122,9 +122,6 @@ static int setup_efi_info_memmap(struct
>  	unsigned long efi_map_phys_addr = params_load_addr + efi_map_offset;
>  	struct efi_info *ei = &params->efi_info;
>  
> -	if (!efi_map_sz)
> -		return 0;
> -
>  	efi_runtime_map_copy(efi_map, efi_map_sz);
>  
>  	ei->efi_memmap = efi_map_phys_addr & 0xffffffff;
> @@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
>  	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
>  	 * without efi.
>  	 */
> -	if (efi_enabled(EFI_OLD_MEMMAP))
> +	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_MEMMAP))
>  		return 0;
>  
>  	ei->efi_loader_signature = current_ei->efi_loader_signature;
> @@ -338,7 +335,7 @@ static void *bzImage64_load(struct kimag
>  	struct kexec_entry64_regs regs64;
>  	void *stack;
>  	unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
> -	unsigned int efi_map_offset, efi_map_sz, efi_setup_data_offset;
> +	unsigned int efi_map_offset = 0, efi_map_sz = 0, efi_setup_data_offset = 0;
>  	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
>  				  .top_down = true };
>  	struct kexec_buf pbuf = { .image = image, .buf_min = MIN_PURGATORY_ADDR,
> @@ -397,19 +394,22 @@ static void *bzImage64_load(struct kimag
>  	 * have to create separate segment for each. Keeps things
>  	 * little bit simple
>  	 */
> -	efi_map_sz = efi_get_runtime_map_size();
>  	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
>  				MAX_ELFCOREHDR_STR_LEN;
>  	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
> -	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16) +
> -				sizeof(struct setup_data) +
> -				sizeof(struct efi_setup_data);
> +	kbuf.bufsz = params_cmdline_sz + sizeof(struct setup_data);
> +
> +	/* Now add space for the efi stuff if we have a useable 1:1 mapping. */
> +	if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_MEMMAP)) {
> +		efi_map_sz = efi_get_runtime_map_size();
> +		kbuf.bufsz += ALIGN(efi_map_sz, 16) + sizeof(struct efi_setup_data);
> +		efi_map_offset = params_cmdline_sz;
> +		efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
> +	}
>  
>  	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
>  	if (!params)
>  		return ERR_PTR(-ENOMEM);
> -	efi_map_offset = params_cmdline_sz;
> -	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
>  
>  	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
>  	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;

Thanks
Dave

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-09  4:21 ` Dave Young
@ 2018-08-09  5:05   ` Mike Galbraith
  2018-08-09  7:33   ` Mike Galbraith
  1 sibling, 0 replies; 11+ messages in thread
From: Mike Galbraith @ 2018-08-09  5:05 UTC (permalink / raw)
  To: Dave Young; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml, kexec

On Thu, 2018-08-09 at 12:21 +0800, Dave Young wrote:
> Hi Mike,
> 
> Thanks for the patch!
> On 08/08/18 at 04:03pm, Mike Galbraith wrote:
> > When booting with efi=noruntime, we call efi_runtime_map_copy() while
> > loading the kdump kernel, and trip over a NULL efi.memmap.map.  Avoid
> > that and a useless allocation when the only mapping we can use (1:1)
> > is not available.
> 
> At first glance, efi_get_runtime_map_size should return 0 in case
> noruntime.

I actually made it do that in a separate patch first, and keyed on that
in a second, but then decided to not notice anything odd in efi land
(run Forest run!), and just fix the bug that now bites latest RT due to
it turning efi runtime off by default.

> Also since we are here, would you mind to restructure the bzImage64_load
> function, and try to move all efi related code to setup_efi_state()?
> 
> 
> setup_boot_parameters(struct kimage *image, struct boot_params *params,
>                       unsigned long params_load_addr,
>                       unsigned int efi_map_offset, unsigned int efi_map_sz,
>                       unsigned int efi_setup_data_offset)
> {
> [snip]
> 
> #ifdef CONFIG_EFI
>         /* Setup EFI state */
>         setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
>                         efi_setup_data_offset);
> #endif
> 
> [snip]
> }
> 
> Currently bzImage64_load prepares the efi_map_offset, efi_map_sz,
>  and efi_setup_data_offset and then pass it to setup_boot_parameters and
> setup_efi_state.  It should be better to move those efi_* variables to
> setup_efi_state().
> 
> So we can call setup_efi_state only when efi runtime is enabled.

Yeah, I thought the same, but wanted to keep it dinky.

	-Mike

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-09  4:21 ` Dave Young
  2018-08-09  5:05   ` Mike Galbraith
@ 2018-08-09  7:33   ` Mike Galbraith
  2018-08-09  9:13     ` Dave Young
  1 sibling, 1 reply; 11+ messages in thread
From: Mike Galbraith @ 2018-08-09  7:33 UTC (permalink / raw)
  To: Dave Young; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml, kexec

On Thu, 2018-08-09 at 12:21 +0800, Dave Young wrote:
> Hi Mike,
> 
> Thanks for the patch!
> On 08/08/18 at 04:03pm, Mike Galbraith wrote:
> > When booting with efi=noruntime, we call efi_runtime_map_copy() while
> > loading the kdump kernel, and trip over a NULL efi.memmap.map.  Avoid
> > that and a useless allocation when the only mapping we can use (1:1)
> > is not available.
> 
> At first glance, efi_get_runtime_map_size should return 0 in case
> noruntime.

What efi does internally at unmap time is to leave everything except
efi.mmap.map untouched, setting it to NULL and turning off EFI_MEMMAP,
rendering efi.mmap.map accessors useless/unsafe without first checking
EFI_MEMMAP.

	-Mike

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-09  7:33   ` Mike Galbraith
@ 2018-08-09  9:13     ` Dave Young
  0 siblings, 0 replies; 11+ messages in thread
From: Dave Young @ 2018-08-09  9:13 UTC (permalink / raw)
  To: Mike Galbraith
  Cc: Baoquan He, Sebastian Andrzej Siewior, lkml, kexec, linux-efi,
	ard.biesheuvel

On 08/09/18 at 09:33am, Mike Galbraith wrote:
> On Thu, 2018-08-09 at 12:21 +0800, Dave Young wrote:
> > Hi Mike,
> > 
> > Thanks for the patch!
> > On 08/08/18 at 04:03pm, Mike Galbraith wrote:
> > > When booting with efi=noruntime, we call efi_runtime_map_copy() while
> > > loading the kdump kernel, and trip over a NULL efi.memmap.map.  Avoid
> > > that and a useless allocation when the only mapping we can use (1:1)
> > > is not available.
> > 
> > At first glance, efi_get_runtime_map_size should return 0 in case
> > noruntime.
> 
> What efi does internally at unmap time is to leave everything except
> efi.mmap.map untouched, setting it to NULL and turning off EFI_MEMMAP,
> rendering efi.mmap.map accessors useless/unsafe without first checking
> EFI_MEMMAP.

Probably the x86 efi_init should reset nr_map to zero in case runtime is
disabled.  But let's see how Ard thinks about this and cc linux-efi.

As for efi_get_runtime_map_size, it was introduced for x86 kexec use.
for copying runtime maps,  so I think it is reasonable this function
return zero in case no runtime.

Thanks
Dave

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-08 14:03 [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference Mike Galbraith
  2018-08-09  4:21 ` Dave Young
@ 2018-08-10  8:45 ` Dave Young
  2018-08-10 10:23   ` Mike Galbraith
  2018-08-10 10:28   ` Dave Young
  1 sibling, 2 replies; 11+ messages in thread
From: Dave Young @ 2018-08-10  8:45 UTC (permalink / raw)
  To: Mike Galbraith; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml

On 08/08/18 at 04:03pm, Mike Galbraith wrote:
> When booting with efi=noruntime, we call efi_runtime_map_copy() while
> loading the kdump kernel, and trip over a NULL efi.memmap.map.  Avoid
> that and a useless allocation when the only mapping we can use (1:1)
> is not available.
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> ---
>  arch/x86/kernel/kexec-bzimage64.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -122,9 +122,6 @@ static int setup_efi_info_memmap(struct
>  	unsigned long efi_map_phys_addr = params_load_addr + efi_map_offset;
>  	struct efi_info *ei = &params->efi_info;
>  
> -	if (!efi_map_sz)
> -		return 0;
> -
>  	efi_runtime_map_copy(efi_map, efi_map_sz);
>  
>  	ei->efi_memmap = efi_map_phys_addr & 0xffffffff;
> @@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
>  	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
>  	 * without efi.
>  	 */
> -	if (efi_enabled(EFI_OLD_MEMMAP))
> +	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_MEMMAP))
>  		return 0;
>  
>  	ei->efi_loader_signature = current_ei->efi_loader_signature;
> @@ -338,7 +335,7 @@ static void *bzImage64_load(struct kimag
>  	struct kexec_entry64_regs regs64;
>  	void *stack;
>  	unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
> -	unsigned int efi_map_offset, efi_map_sz, efi_setup_data_offset;
> +	unsigned int efi_map_offset = 0, efi_map_sz = 0, efi_setup_data_offset = 0;
>  	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
>  				  .top_down = true };
>  	struct kexec_buf pbuf = { .image = image, .buf_min = MIN_PURGATORY_ADDR,
> @@ -397,19 +394,22 @@ static void *bzImage64_load(struct kimag
>  	 * have to create separate segment for each. Keeps things
>  	 * little bit simple
>  	 */
> -	efi_map_sz = efi_get_runtime_map_size();
>  	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
>  				MAX_ELFCOREHDR_STR_LEN;
>  	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
> -	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16) +
> -				sizeof(struct setup_data) +
> -				sizeof(struct efi_setup_data);
> +	kbuf.bufsz = params_cmdline_sz + sizeof(struct setup_data);
> +
> +	/* Now add space for the efi stuff if we have a useable 1:1 mapping. */
> +	if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_MEMMAP)) {
> +		efi_map_sz = efi_get_runtime_map_size();
> +		kbuf.bufsz += ALIGN(efi_map_sz, 16) + sizeof(struct efi_setup_data);
> +		efi_map_offset = params_cmdline_sz;
> +		efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
> +	}
>  
>  	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
>  	if (!params)
>  		return ERR_PTR(-ENOMEM);
> -	efi_map_offset = params_cmdline_sz;
> -	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
>  
>  	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
>  	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;

BTW, this patch only fix the kexec load phase problem,  even if kexec
load successfully with the fix, the 2nd kernel can not boot because efi
memmap info is not correct and usable.

So we should go with some fix similar to below, and do the cleanup we
mentioned with a separate patch later.

Also user space kexec-tools need a similar patch to error out in case
no runtime maps.  It would be good to fix both userspace and kernel
load.

diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
index 7326078eaa7a..e34ba2f53cfb 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -123,7 +123,7 @@ static int setup_efi_info_memmap(struct boot_params *params,
 	struct efi_info *ei = &params->efi_info;
 
 	if (!efi_map_sz)
-		return 0;
+		return -EINVAL;
 
 	efi_runtime_map_copy(efi_map, efi_map_sz);
 
@@ -166,9 +166,10 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
 {
 	struct efi_info *current_ei = &boot_params.efi_info;
 	struct efi_info *ei = &params->efi_info;
+	int ret;
 
 	if (!current_ei->efi_memmap_size)
-		return 0;
+		return -EINVAL;
 
 	/*
 	 * If 1:1 mapping is not enabled, second kernel can not setup EFI
@@ -176,8 +177,8 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
 	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
 	 * without efi.
 	 */
-	if (efi_enabled(EFI_OLD_MEMMAP))
-		return 0;
+	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_RUNTIME_SERVICES))
+		return -ENODEV;
 
 	ei->efi_loader_signature = current_ei->efi_loader_signature;
 	ei->efi_systab = current_ei->efi_systab;
@@ -186,8 +187,10 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
 	ei->efi_memdesc_version = current_ei->efi_memdesc_version;
 	ei->efi_memdesc_size = efi_get_runtime_map_desc_size();
 
-	setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
+	ret = setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
 			      efi_map_sz);
+	if (ret)
+		return ret;
 	prepare_add_efi_setup_data(params, params_load_addr,
 				   efi_setup_data_offset);
 	return 0;
@@ -250,8 +253,10 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
 
 #ifdef CONFIG_EFI
 	/* Setup EFI state */
-	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
+	ret = setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
 			efi_setup_data_offset);
+	if (ret)
+		return ret;
 #endif
 
 	/* Setup EDD info */

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-10  8:45 ` Dave Young
@ 2018-08-10 10:23   ` Mike Galbraith
  2018-08-10 10:28   ` Dave Young
  1 sibling, 0 replies; 11+ messages in thread
From: Mike Galbraith @ 2018-08-10 10:23 UTC (permalink / raw)
  To: Dave Young; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml

On Fri, 2018-08-10 at 16:45 +0800, Dave Young wrote:
> 
> BTW, this patch only fix the kexec load phase problem,  even if kexec
> load successfully with the fix, the 2nd kernel can not boot because efi
> memmap info is not correct and usable.

Hm.  I didn't do anything else with kexec, but did crashdump my box
both w/wo efi=noruntime.

> So we should go with some fix similar to below, and do the cleanup we
> mentioned with a separate patch later.

Ah, you mean the one I had _just_ built when I saw this :)

> Also user space kexec-tools need a similar patch to error out in case
> no runtime maps.  It would be good to fix both userspace and kernel
> load.
> 
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 7326078eaa7a..e34ba2f53cfb 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -123,7 +123,7 @@ static int setup_efi_info_memmap(struct boot_params *params,
>  	struct efi_info *ei = &params->efi_info;
>  
>  	if (!efi_map_sz)
> -		return 0;
> +		return -EINVAL;
>  
>  	efi_runtime_map_copy(efi_map, efi_map_sz);
>  
> @@ -166,9 +166,10 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  {
>  	struct efi_info *current_ei = &boot_params.efi_info;
>  	struct efi_info *ei = &params->efi_info;
> +	int ret;
>  
>  	if (!current_ei->efi_memmap_size)
> -		return 0;
> +		return -EINVAL;
>  
>  	/*
>  	 * If 1:1 mapping is not enabled, second kernel can not setup EFI
> @@ -176,8 +177,8 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
>  	 * without efi.
>  	 */
> -	if (efi_enabled(EFI_OLD_MEMMAP))
> -		return 0;
> +	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_RUNTIME_SERVICES))
> +		return -ENODEV;
>  
>  	ei->efi_loader_signature = current_ei->efi_loader_signature;
>  	ei->efi_systab = current_ei->efi_systab;
> @@ -186,8 +187,10 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  	ei->efi_memdesc_version = current_ei->efi_memdesc_version;
>  	ei->efi_memdesc_size = efi_get_runtime_map_desc_size();
>  
> -	setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
> +	ret = setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
>  			      efi_map_sz);
> +	if (ret)
> +		return ret;
>  	prepare_add_efi_setup_data(params, params_load_addr,
>  				   efi_setup_data_offset);
>  	return 0;
> @@ -250,8 +253,10 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
>  
>  #ifdef CONFIG_EFI
>  	/* Setup EFI state */
> -	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> +	ret = setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
>  			efi_setup_data_offset);
> +	if (ret)
> +		return ret;
>  #endif
>  
>  	/* Setup EDD info */

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-10  8:45 ` Dave Young
  2018-08-10 10:23   ` Mike Galbraith
@ 2018-08-10 10:28   ` Dave Young
  2018-08-10 17:39     ` Mike Galbraith
  1 sibling, 1 reply; 11+ messages in thread
From: Dave Young @ 2018-08-10 10:28 UTC (permalink / raw)
  To: Mike Galbraith; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml

On 08/10/18 at 04:45pm, Dave Young wrote:
> On 08/08/18 at 04:03pm, Mike Galbraith wrote:
> > When booting with efi=noruntime, we call efi_runtime_map_copy() while
> > loading the kdump kernel, and trip over a NULL efi.memmap.map.  Avoid
> > that and a useless allocation when the only mapping we can use (1:1)
> > is not available.
> > 
> > Signed-off-by: Mike Galbraith <efault@gmx.de>
> > ---
> >  arch/x86/kernel/kexec-bzimage64.c |   22 +++++++++++-----------
> >  1 file changed, 11 insertions(+), 11 deletions(-)
> > 
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -122,9 +122,6 @@ static int setup_efi_info_memmap(struct
> >  	unsigned long efi_map_phys_addr = params_load_addr + efi_map_offset;
> >  	struct efi_info *ei = &params->efi_info;
> >  
> > -	if (!efi_map_sz)
> > -		return 0;
> > -
> >  	efi_runtime_map_copy(efi_map, efi_map_sz);
> >  
> >  	ei->efi_memmap = efi_map_phys_addr & 0xffffffff;
> > @@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
> >  	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
> >  	 * without efi.
> >  	 */
> > -	if (efi_enabled(EFI_OLD_MEMMAP))
> > +	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_MEMMAP))
> >  		return 0;
> >  
> >  	ei->efi_loader_signature = current_ei->efi_loader_signature;
> > @@ -338,7 +335,7 @@ static void *bzImage64_load(struct kimag
> >  	struct kexec_entry64_regs regs64;
> >  	void *stack;
> >  	unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
> > -	unsigned int efi_map_offset, efi_map_sz, efi_setup_data_offset;
> > +	unsigned int efi_map_offset = 0, efi_map_sz = 0, efi_setup_data_offset = 0;
> >  	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
> >  				  .top_down = true };
> >  	struct kexec_buf pbuf = { .image = image, .buf_min = MIN_PURGATORY_ADDR,
> > @@ -397,19 +394,22 @@ static void *bzImage64_load(struct kimag
> >  	 * have to create separate segment for each. Keeps things
> >  	 * little bit simple
> >  	 */
> > -	efi_map_sz = efi_get_runtime_map_size();
> >  	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
> >  				MAX_ELFCOREHDR_STR_LEN;
> >  	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
> > -	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16) +
> > -				sizeof(struct setup_data) +
> > -				sizeof(struct efi_setup_data);
> > +	kbuf.bufsz = params_cmdline_sz + sizeof(struct setup_data);
> > +
> > +	/* Now add space for the efi stuff if we have a useable 1:1 mapping. */
> > +	if (!efi_enabled(EFI_OLD_MEMMAP) && efi_enabled(EFI_MEMMAP)) {
> > +		efi_map_sz = efi_get_runtime_map_size();
> > +		kbuf.bufsz += ALIGN(efi_map_sz, 16) + sizeof(struct efi_setup_data);
> > +		efi_map_offset = params_cmdline_sz;
> > +		efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
> > +	}
> >  
> >  	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
> >  	if (!params)
> >  		return ERR_PTR(-ENOMEM);
> > -	efi_map_offset = params_cmdline_sz;
> > -	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
> >  
> >  	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
> >  	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;
> 
> BTW, this patch only fix the kexec load phase problem,  even if kexec
> load successfully with the fix, the 2nd kernel can not boot because efi
> memmap info is not correct and usable.
> 
> So we should go with some fix similar to below, and do the cleanup we
> mentioned with a separate patch later.
> 
> Also user space kexec-tools need a similar patch to error out in case
> no runtime maps.  It would be good to fix both userspace and kernel
> load.
> 
> diff --git a/arch/x86/kernel/kexec-bzimage64.c b/arch/x86/kernel/kexec-bzimage64.c
> index 7326078eaa7a..e34ba2f53cfb 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -123,7 +123,7 @@ static int setup_efi_info_memmap(struct boot_params *params,
>  	struct efi_info *ei = &params->efi_info;
>  
>  	if (!efi_map_sz)
> -		return 0;
> +		return -EINVAL;
>  
>  	efi_runtime_map_copy(efi_map, efi_map_sz);
>  
> @@ -166,9 +166,10 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  {
>  	struct efi_info *current_ei = &boot_params.efi_info;
>  	struct efi_info *ei = &params->efi_info;
> +	int ret;
>  
>  	if (!current_ei->efi_memmap_size)
> -		return 0;
> +		return -EINVAL;
>  
>  	/*
>  	 * If 1:1 mapping is not enabled, second kernel can not setup EFI
> @@ -176,8 +177,8 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
>  	 * without efi.
>  	 */
> -	if (efi_enabled(EFI_OLD_MEMMAP))
> -		return 0;
> +	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_RUNTIME_SERVICES))
> +		return -ENODEV;
>  
>  	ei->efi_loader_signature = current_ei->efi_loader_signature;
>  	ei->efi_systab = current_ei->efi_systab;
> @@ -186,8 +187,10 @@ setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
>  	ei->efi_memdesc_version = current_ei->efi_memdesc_version;
>  	ei->efi_memdesc_size = efi_get_runtime_map_desc_size();
>  
> -	setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
> +	ret = setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
>  			      efi_map_sz);
> +	if (ret)
> +		return ret;
>  	prepare_add_efi_setup_data(params, params_load_addr,
>  				   efi_setup_data_offset);
>  	return 0;
> @@ -250,8 +253,10 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
>  
>  #ifdef CONFIG_EFI
>  	/* Setup EFI state */
> -	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> +	ret = setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
>  			efi_setup_data_offset);
> +	if (ret)

Here should check efi_enabled(EFI_BOOT) && ret

In case efi boot we need the efi info set correctly,  or one need pass
acpi_rsdp= in kernel cmdline param.

Still not sure how to allow one to workaround it by using acpi_rsdp=
param with kexec_file_load.. 


> +		return ret;
>  #endif
>  
>  	/* Setup EDD info */

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-10 10:28   ` Dave Young
@ 2018-08-10 17:39     ` Mike Galbraith
  2018-08-15  3:59       ` Dave Young
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Galbraith @ 2018-08-10 17:39 UTC (permalink / raw)
  To: Dave Young; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml

On Fri, 2018-08-10 at 18:28 +0800, Dave Young wrote:
> 
> > @@ -250,8 +253,10 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> >  
> >  #ifdef CONFIG_EFI
> >  	/* Setup EFI state */
> > -	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> > +	ret = setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> >  			efi_setup_data_offset);
> > +	if (ret)
> 
> Here should check efi_enabled(EFI_BOOT) && ret

Patch with that works for me.

> In case efi boot we need the efi info set correctly,  or one need pass
> acpi_rsdp= in kernel cmdline param.
> 
> Still not sure how to allow one to workaround it by using acpi_rsdp=
> param with kexec_file_load..

Does this improve things, and plug the no boot hole?

x86, kdump: cleanup efi setup data handling a bit

1. Remove efi specific variables from bzImage64_load() other than the
one it needs, efi_map_sz, passing it and params_cmdline_sz on to efi
setup functions, giving them all they need without duplication.

2. Only allocate space for efi setup data when a 1:1 mapping is available.
Bail early with -ENODEV if not available, but is required to boot, and
acpi_rsdp= was not passed on the command line. 

3. Use the proper config dependency to isolate efi setup functions,
adding a !EFI_RUNTIME_MAP stub for setup_efi_state().

4. Change efi functions that cannot fail to void. 

Signed-off-by: Mike Galbraith <efault@gmx.de>
---
 arch/x86/kernel/kexec-bzimage64.c |   99 +++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 54 deletions(-)

--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -112,35 +112,32 @@ static int setup_e820_entries(struct boo
 	return 0;
 }
 
-#ifdef CONFIG_EFI
-static int setup_efi_info_memmap(struct boot_params *params,
+#ifdef CONFIG_EFI_RUNTIME_MAP
+static void setup_efi_info_memmap(struct boot_params *params,
 				  unsigned long params_load_addr,
-				  unsigned int efi_map_offset,
+				  unsigned int params_cmdline_sz,
 				  unsigned int efi_map_sz)
 {
-	void *efi_map = (void *)params + efi_map_offset;
-	unsigned long efi_map_phys_addr = params_load_addr + efi_map_offset;
+	void *efi_map = (void *)params + params_cmdline_sz;
+	unsigned long efi_map_phys_addr = params_load_addr + params_cmdline_sz;
 	struct efi_info *ei = &params->efi_info;
 
-	if (!efi_map_sz)
-		return -EINVAL;
-
 	efi_runtime_map_copy(efi_map, efi_map_sz);
 
 	ei->efi_memmap = efi_map_phys_addr & 0xffffffff;
 	ei->efi_memmap_hi = efi_map_phys_addr >> 32;
 	ei->efi_memmap_size = efi_map_sz;
-
-	return 0;
 }
 
-static int
+static void
 prepare_add_efi_setup_data(struct boot_params *params,
-		       unsigned long params_load_addr,
-		       unsigned int efi_setup_data_offset)
+			   unsigned long params_load_addr,
+			   unsigned int params_cmdline_sz,
+			   unsigned int efi_map_sz)
 {
+	unsigned int data_offset = params_cmdline_sz + ALIGN(efi_map_sz, 16);
 	unsigned long setup_data_phys;
-	struct setup_data *sd = (void *)params + efi_setup_data_offset;
+	struct setup_data *sd = (void *)params + data_offset;
 	struct efi_setup_data *esd = (void *)sd + sizeof(struct setup_data);
 
 	esd->fw_vendor = efi.fw_vendor;
@@ -152,33 +149,20 @@ prepare_add_efi_setup_data(struct boot_p
 	sd->len = sizeof(struct efi_setup_data);
 
 	/* Add setup data */
-	setup_data_phys = params_load_addr + efi_setup_data_offset;
+	setup_data_phys = params_load_addr + data_offset;
 	sd->next = params->hdr.setup_data;
 	params->hdr.setup_data = setup_data_phys;
-
-	return 0;
 }
 
 static int
 setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
-		unsigned int efi_map_offset, unsigned int efi_map_sz,
-		unsigned int efi_setup_data_offset)
+		unsigned int params_cmdline_sz, unsigned int efi_map_sz)
 {
 	struct efi_info *current_ei = &boot_params.efi_info;
 	struct efi_info *ei = &params->efi_info;
-	int ret;
-
-	if (!current_ei->efi_memmap_size)
-		return -EINVAL;
 
-	/*
-	 * If 1:1 mapping is not enabled, second kernel can not setup EFI
-	 * and use EFI run time services. User space will have to pass
-	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
-	 * without efi.
-	 */
-	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_RUNTIME_SERVICES))
-		return -ENODEV;
+	if (!efi_map_sz || !current_ei->efi_memmap_size)
+		return efi_map_sz ? -EINVAL : 0;
 
 	ei->efi_loader_signature = current_ei->efi_loader_signature;
 	ei->efi_systab = current_ei->efi_systab;
@@ -187,21 +171,24 @@ setup_efi_state(struct boot_params *para
 	ei->efi_memdesc_version = current_ei->efi_memdesc_version;
 	ei->efi_memdesc_size = efi_get_runtime_map_desc_size();
 
-	ret = setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
+	setup_efi_info_memmap(params, params_load_addr, params_cmdline_sz,
 			      efi_map_sz);
-	if (ret)
-		return ret;
-	prepare_add_efi_setup_data(params, params_load_addr,
-				   efi_setup_data_offset);
+	prepare_add_efi_setup_data(params, params_load_addr, params_cmdline_sz,
+				   efi_map_sz);
 	return 0;
 }
-#endif /* CONFIG_EFI */
+#else /* !CONFIG_EFI_RUNTIME_MAP */
+static int
+setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
+		unsigned int params_cmdline_sz, unsigned int efi_map_sz)
+{ return 0; }
+#endif /* CONFIG_EFI_RUNTIME_MAP */
 
 static int
 setup_boot_parameters(struct kimage *image, struct boot_params *params,
 		      unsigned long params_load_addr,
-		      unsigned int efi_map_offset, unsigned int efi_map_sz,
-		      unsigned int efi_setup_data_offset)
+		      unsigned int params_cmdline_sz,
+		      unsigned int efi_map_sz)
 {
 	unsigned int nr_e820_entries;
 	unsigned long long mem_k, start, end;
@@ -251,13 +238,9 @@ setup_boot_parameters(struct kimage *ima
 		}
 	}
 
-#ifdef CONFIG_EFI
-	/* Setup EFI state */
-	ret = setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
-			efi_setup_data_offset);
-	if (efi_enabled(EFI_BOOT) && ret)
+	ret = setup_efi_state(params, params_load_addr, params_cmdline_sz, efi_map_sz);
+	if (ret)
 		return ret;
-#endif
 
 	/* Setup EDD info */
 	memcpy(params->eddbuf, boot_params.eddbuf,
@@ -343,7 +326,7 @@ static void *bzImage64_load(struct kimag
 	struct kexec_entry64_regs regs64;
 	void *stack;
 	unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
-	unsigned int efi_map_offset, efi_map_sz, efi_setup_data_offset;
+	unsigned int efi_map_sz = 0;
 	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
 				  .top_down = true };
 	struct kexec_buf pbuf = { .image = image, .buf_min = MIN_PURGATORY_ADDR,
@@ -402,19 +385,28 @@ static void *bzImage64_load(struct kimag
 	 * have to create separate segment for each. Keeps things
 	 * little bit simple
 	 */
-	efi_map_sz = efi_get_runtime_map_size();
 	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
 				MAX_ELFCOREHDR_STR_LEN;
 	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
-	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16) +
-				sizeof(struct setup_data) +
-				sizeof(struct efi_setup_data);
+	kbuf.bufsz = params_cmdline_sz + sizeof(struct setup_data);
+
+	/*
+	 * If a 1:1 mapping does not exist, the second kernel cannot setup
+	 * and use EFI run time services, user space will have to pass
+	 * acpi_rsdp=<addr> on the kernel command line to make the second
+	 * kernel boot without efi.  Allocate space for efi setup data if
+	 * this constraint is met, bail if not, but is required to boot,
+	 * and acpi_rsdp=<addr> was not passed on the command line.
+	 */
+	if (efi_enabled(EFI_RUNTIME_SERVICES) && !efi_enabled(EFI_OLD_MEMMAP)) {
+		efi_map_sz = efi_get_runtime_map_size();
+		kbuf.bufsz += ALIGN(efi_map_sz, 16) + sizeof(struct efi_setup_data);
+	} else if (efi_enabled(EFI_BOOT) && !strstr(cmdline, "acpi_rsdp="))
+		return ERR_PTR(-ENODEV);
 
 	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
 	if (!params)
 		return ERR_PTR(-ENOMEM);
-	efi_map_offset = params_cmdline_sz;
-	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
 
 	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
 	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;
@@ -494,8 +486,7 @@ static void *bzImage64_load(struct kimag
 		goto out_free_params;
 
 	ret = setup_boot_parameters(image, params, bootparam_load_addr,
-				    efi_map_offset, efi_map_sz,
-				    efi_setup_data_offset);
+				    params_cmdline_sz, efi_map_sz);
 	if (ret)
 		goto out_free_params;
 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-10 17:39     ` Mike Galbraith
@ 2018-08-15  3:59       ` Dave Young
  2018-08-15  4:57         ` Mike Galbraith
  0 siblings, 1 reply; 11+ messages in thread
From: Dave Young @ 2018-08-15  3:59 UTC (permalink / raw)
  To: Mike Galbraith; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml

Apologize for late reply, I'm occupied with something else.

On 08/10/18 at 07:39pm, Mike Galbraith wrote:
> On Fri, 2018-08-10 at 18:28 +0800, Dave Young wrote:
> > 
> > > @@ -250,8 +253,10 @@ setup_boot_parameters(struct kimage *image, struct boot_params *params,
> > >  
> > >  #ifdef CONFIG_EFI
> > >  	/* Setup EFI state */
> > > -	setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> > > +	ret = setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> > >  			efi_setup_data_offset);
> > > +	if (ret)
> > 
> > Here should check efi_enabled(EFI_BOOT) && ret
> 
> Patch with that works for me.
> 
> > In case efi boot we need the efi info set correctly,  or one need pass
> > acpi_rsdp= in kernel cmdline param.
> > 
> > Still not sure how to allow one to workaround it by using acpi_rsdp=
> > param with kexec_file_load..
> 
> Does this improve things, and plug the no boot hole?

Would you mind to tune my patch with some acpi_rsdp checking and add
some error message in case kexec load failure? Eg. suggest people to use
append acpi_rsdp for noefi booting etc.

I'm still not very satisfied with the code cleanup, ideally we should add a
separate kbuf for efi stuff, so that we can isolate the efi_map_sz
efi_setup_data_offset, and efi_map_offset initialization only when
necessary.  Anyway the cleanup can be a separate patch.

> 
> x86, kdump: cleanup efi setup data handling a bit
> 
> 1. Remove efi specific variables from bzImage64_load() other than the
> one it needs, efi_map_sz, passing it and params_cmdline_sz on to efi
> setup functions, giving them all they need without duplication.
> 
> 2. Only allocate space for efi setup data when a 1:1 mapping is available.
> Bail early with -ENODEV if not available, but is required to boot, and
> acpi_rsdp= was not passed on the command line. 
> 
> 3. Use the proper config dependency to isolate efi setup functions,
> adding a !EFI_RUNTIME_MAP stub for setup_efi_state().
> 
> 4. Change efi functions that cannot fail to void. 
> 
> Signed-off-by: Mike Galbraith <efault@gmx.de>
> ---
>  arch/x86/kernel/kexec-bzimage64.c |   99 +++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 54 deletions(-)
> 
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -112,35 +112,32 @@ static int setup_e820_entries(struct boo
>  	return 0;
>  }
>  
> -#ifdef CONFIG_EFI
> -static int setup_efi_info_memmap(struct boot_params *params,
> +#ifdef CONFIG_EFI_RUNTIME_MAP
> +static void setup_efi_info_memmap(struct boot_params *params,
>  				  unsigned long params_load_addr,
> -				  unsigned int efi_map_offset,
> +				  unsigned int params_cmdline_sz,
>  				  unsigned int efi_map_sz)
>  {
> -	void *efi_map = (void *)params + efi_map_offset;
> -	unsigned long efi_map_phys_addr = params_load_addr + efi_map_offset;
> +	void *efi_map = (void *)params + params_cmdline_sz;
> +	unsigned long efi_map_phys_addr = params_load_addr + params_cmdline_sz;
>  	struct efi_info *ei = &params->efi_info;
>  
> -	if (!efi_map_sz)
> -		return -EINVAL;
> -
>  	efi_runtime_map_copy(efi_map, efi_map_sz);
>  
>  	ei->efi_memmap = efi_map_phys_addr & 0xffffffff;
>  	ei->efi_memmap_hi = efi_map_phys_addr >> 32;
>  	ei->efi_memmap_size = efi_map_sz;
> -
> -	return 0;
>  }
>  
> -static int
> +static void
>  prepare_add_efi_setup_data(struct boot_params *params,
> -		       unsigned long params_load_addr,
> -		       unsigned int efi_setup_data_offset)
> +			   unsigned long params_load_addr,
> +			   unsigned int params_cmdline_sz,
> +			   unsigned int efi_map_sz)
>  {
> +	unsigned int data_offset = params_cmdline_sz + ALIGN(efi_map_sz, 16);
>  	unsigned long setup_data_phys;
> -	struct setup_data *sd = (void *)params + efi_setup_data_offset;
> +	struct setup_data *sd = (void *)params + data_offset;
>  	struct efi_setup_data *esd = (void *)sd + sizeof(struct setup_data);
>  
>  	esd->fw_vendor = efi.fw_vendor;
> @@ -152,33 +149,20 @@ prepare_add_efi_setup_data(struct boot_p
>  	sd->len = sizeof(struct efi_setup_data);
>  
>  	/* Add setup data */
> -	setup_data_phys = params_load_addr + efi_setup_data_offset;
> +	setup_data_phys = params_load_addr + data_offset;
>  	sd->next = params->hdr.setup_data;
>  	params->hdr.setup_data = setup_data_phys;
> -
> -	return 0;
>  }
>  
>  static int
>  setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
> -		unsigned int efi_map_offset, unsigned int efi_map_sz,
> -		unsigned int efi_setup_data_offset)
> +		unsigned int params_cmdline_sz, unsigned int efi_map_sz)
>  {
>  	struct efi_info *current_ei = &boot_params.efi_info;
>  	struct efi_info *ei = &params->efi_info;
> -	int ret;
> -
> -	if (!current_ei->efi_memmap_size)
> -		return -EINVAL;
>  
> -	/*
> -	 * If 1:1 mapping is not enabled, second kernel can not setup EFI
> -	 * and use EFI run time services. User space will have to pass
> -	 * acpi_rsdp=<addr> on kernel command line to make second kernel boot
> -	 * without efi.
> -	 */
> -	if (efi_enabled(EFI_OLD_MEMMAP) || !efi_enabled(EFI_RUNTIME_SERVICES))
> -		return -ENODEV;
> +	if (!efi_map_sz || !current_ei->efi_memmap_size)
> +		return efi_map_sz ? -EINVAL : 0;
>  
>  	ei->efi_loader_signature = current_ei->efi_loader_signature;
>  	ei->efi_systab = current_ei->efi_systab;
> @@ -187,21 +171,24 @@ setup_efi_state(struct boot_params *para
>  	ei->efi_memdesc_version = current_ei->efi_memdesc_version;
>  	ei->efi_memdesc_size = efi_get_runtime_map_desc_size();
>  
> -	ret = setup_efi_info_memmap(params, params_load_addr, efi_map_offset,
> +	setup_efi_info_memmap(params, params_load_addr, params_cmdline_sz,
>  			      efi_map_sz);
> -	if (ret)
> -		return ret;
> -	prepare_add_efi_setup_data(params, params_load_addr,
> -				   efi_setup_data_offset);
> +	prepare_add_efi_setup_data(params, params_load_addr, params_cmdline_sz,
> +				   efi_map_sz);
>  	return 0;
>  }
> -#endif /* CONFIG_EFI */
> +#else /* !CONFIG_EFI_RUNTIME_MAP */
> +static int
> +setup_efi_state(struct boot_params *params, unsigned long params_load_addr,
> +		unsigned int params_cmdline_sz, unsigned int efi_map_sz)
> +{ return 0; }
> +#endif /* CONFIG_EFI_RUNTIME_MAP */
>  
>  static int
>  setup_boot_parameters(struct kimage *image, struct boot_params *params,
>  		      unsigned long params_load_addr,
> -		      unsigned int efi_map_offset, unsigned int efi_map_sz,
> -		      unsigned int efi_setup_data_offset)
> +		      unsigned int params_cmdline_sz,
> +		      unsigned int efi_map_sz)
>  {
>  	unsigned int nr_e820_entries;
>  	unsigned long long mem_k, start, end;
> @@ -251,13 +238,9 @@ setup_boot_parameters(struct kimage *ima
>  		}
>  	}
>  
> -#ifdef CONFIG_EFI
> -	/* Setup EFI state */
> -	ret = setup_efi_state(params, params_load_addr, efi_map_offset, efi_map_sz,
> -			efi_setup_data_offset);
> -	if (efi_enabled(EFI_BOOT) && ret)
> +	ret = setup_efi_state(params, params_load_addr, params_cmdline_sz, efi_map_sz);
> +	if (ret)
>  		return ret;
> -#endif
>  
>  	/* Setup EDD info */
>  	memcpy(params->eddbuf, boot_params.eddbuf,
> @@ -343,7 +326,7 @@ static void *bzImage64_load(struct kimag
>  	struct kexec_entry64_regs regs64;
>  	void *stack;
>  	unsigned int setup_hdr_offset = offsetof(struct boot_params, hdr);
> -	unsigned int efi_map_offset, efi_map_sz, efi_setup_data_offset;
> +	unsigned int efi_map_sz = 0;
>  	struct kexec_buf kbuf = { .image = image, .buf_max = ULONG_MAX,
>  				  .top_down = true };
>  	struct kexec_buf pbuf = { .image = image, .buf_min = MIN_PURGATORY_ADDR,
> @@ -402,19 +385,28 @@ static void *bzImage64_load(struct kimag
>  	 * have to create separate segment for each. Keeps things
>  	 * little bit simple
>  	 */
> -	efi_map_sz = efi_get_runtime_map_size();
>  	params_cmdline_sz = sizeof(struct boot_params) + cmdline_len +
>  				MAX_ELFCOREHDR_STR_LEN;
>  	params_cmdline_sz = ALIGN(params_cmdline_sz, 16);
> -	kbuf.bufsz = params_cmdline_sz + ALIGN(efi_map_sz, 16) +
> -				sizeof(struct setup_data) +
> -				sizeof(struct efi_setup_data);
> +	kbuf.bufsz = params_cmdline_sz + sizeof(struct setup_data);
> +
> +	/*
> +	 * If a 1:1 mapping does not exist, the second kernel cannot setup
> +	 * and use EFI run time services, user space will have to pass
> +	 * acpi_rsdp=<addr> on the kernel command line to make the second
> +	 * kernel boot without efi.  Allocate space for efi setup data if
> +	 * this constraint is met, bail if not, but is required to boot,
> +	 * and acpi_rsdp=<addr> was not passed on the command line.
> +	 */
> +	if (efi_enabled(EFI_RUNTIME_SERVICES) && !efi_enabled(EFI_OLD_MEMMAP)) {
> +		efi_map_sz = efi_get_runtime_map_size();
> +		kbuf.bufsz += ALIGN(efi_map_sz, 16) + sizeof(struct efi_setup_data);
> +	} else if (efi_enabled(EFI_BOOT) && !strstr(cmdline, "acpi_rsdp="))
> +		return ERR_PTR(-ENODEV);
>  
>  	params = kzalloc(kbuf.bufsz, GFP_KERNEL);
>  	if (!params)
>  		return ERR_PTR(-ENOMEM);
> -	efi_map_offset = params_cmdline_sz;
> -	efi_setup_data_offset = efi_map_offset + ALIGN(efi_map_sz, 16);
>  
>  	/* Copy setup header onto bootparams. Documentation/x86/boot.txt */
>  	setup_header_size = 0x0202 + kernel[0x0201] - setup_hdr_offset;
> @@ -494,8 +486,7 @@ static void *bzImage64_load(struct kimag
>  		goto out_free_params;
>  
>  	ret = setup_boot_parameters(image, params, bootparam_load_addr,
> -				    efi_map_offset, efi_map_sz,
> -				    efi_setup_data_offset);
> +				    params_cmdline_sz, efi_map_sz);
>  	if (ret)
>  		goto out_free_params;
>  
> 

Thanks
Dave

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
  2018-08-15  3:59       ` Dave Young
@ 2018-08-15  4:57         ` Mike Galbraith
  0 siblings, 0 replies; 11+ messages in thread
From: Mike Galbraith @ 2018-08-15  4:57 UTC (permalink / raw)
  To: Dave Young; +Cc: Baoquan He, Sebastian Andrzej Siewior, lkml

On Wed, 2018-08-15 at 11:59 +0800, Dave Young wrote:
> > Does this improve things, and plug the no boot hole?
> 
> Would you mind to tune my patch with some acpi_rsdp checking and add
> some error message in case kexec load failure? Eg. suggest people to use
> append acpi_rsdp for noefi booting etc.

Yeah, -ENODEV is better than hanging, but not very informative.

> I'm still not very satisfied with the code cleanup..

Not surprising, I didn't like it much either (ergo interrogative).

	-Mike

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-08 14:03 [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference Mike Galbraith
2018-08-09  4:21 ` Dave Young
2018-08-09  5:05   ` Mike Galbraith
2018-08-09  7:33   ` Mike Galbraith
2018-08-09  9:13     ` Dave Young
2018-08-10  8:45 ` Dave Young
2018-08-10 10:23   ` Mike Galbraith
2018-08-10 10:28   ` Dave Young
2018-08-10 17:39     ` Mike Galbraith
2018-08-15  3:59       ` Dave Young
2018-08-15  4:57         ` Mike Galbraith

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox