x86, kdump: Fix efi=noruntime NULL pointer dereference

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 <[email protected]>

---
 arch/x86/kernel/kexec-bzimage64.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

arch/x86/kernel/kexec-bzimage64.c +10 -9

@@ -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);

Hi Mike,

Thanks for the patch!

Quoted 4 lines · Mike Galbraith

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.

Hi Mike,

Thanks for the patch!

Quoted 4 lines · Mike Galbraith

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.

Quoted 9 lines · Dave Young

Hi Mike,

Thanks for the patch!

Quoted 4 lines · Mike Galbraith

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.

Quoted 2 lines

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.

Quoted 10 lines · Mike Galbraith

Quoted 9 lines · Dave Young

Hi Mike,

Thanks for the patch!

Quoted 4 lines · Mike Galbraith

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 22 lines · Dave Young

Quoted 10 lines · Mike Galbraith

Quoted 9 lines · Dave Young

Hi Mike,

Thanks for the patch!

Quoted 4 lines · Mike Galbraith

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 4 lines

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.

I don't see the patch in the context so I cannot comment in great detail.

In any case, it is better to decouple EFI_MEMMAP from EFI_RUNTIME dependencies. On x86, one may imply the other, but this is not generally the case.

That means that efi_get_runtime_map_size() should probably check the EFI_RUNTIME flag, and return 0 if it is cleared. Perhaps there are other places where EFI_MEMMAP flag checks are missing, but I consider that a separate issue.

Quoted 23 lines · Ard Biesheuvel

Quoted 22 lines · Dave Young

Quoted 10 lines · Mike Galbraith

Quoted 9 lines · Dave Young

Hi Mike,

Thanks for the patch!

Quoted 4 lines · Mike Galbraith

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

I don't see the patch in the context so I cannot comment in great detail.

The patch is below:
https://lore.kernel.org/lkml/[email protected]

Quoted 9 lines

In any case, it is better to decouple EFI_MEMMAP from EFI_RUNTIME dependencies. On x86, one may imply the other, but this is not generally the case.

That means that efi_get_runtime_map_size() should probably check the EFI_RUNTIME flag, and return 0 if it is cleared. Perhaps there are other places where EFI_MEMMAP flag checks are missing, but I consider that a separate issue.

Yes, I also agree with to check EFI_RUNTIME_SERVICES. There is no point for efi_get_runtime_map_size to return a value other than 0 in case EFI_RUNTIME_SERVICES is not set ie. via efi=noruntime

Is below patch acceptable? The copy function can be changed to return an error in case map size == 0, but that can be done later along with the caller size cleanups in kexec code

---------------------------------------------------------------------------

efi: check EFI_RUNTIME_SERVICES flag in runtime map copying code

Mike reported a kexec_file_load NULL pointer dereference bug like below:

[    5.878262] BUG: unable to handle kernel NULL pointer dereference at 0000000000000000
[    5.879868] PGD 800000013c1f1067 P4D 800000013c1f1067 PUD 13aea7067 PMD 0 
[    5.881225] Oops: 0000 [#1] SMP PTI
[    5.882068] Modules linked in:
[    5.882851] CPU: 0 PID: 394 Comm: kexec Kdump: loaded Not tainted 4.17.0-rc2+ #648
[    5.884333] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
[    5.885843] RIP: 0010:memcpy_erms+0x6/0x10
[    5.886789] RSP: 0018:ffffc9000058bd00 EFLAGS: 00010246
[    5.887899] RAX: ffff880138e050b0 RBX: 00000000000980b0 RCX: 0000000000000ba0
[    5.889304] RDX: 0000000000000ba0 RSI: 0000000000000000 RDI: ffff880138e050b0
[    5.890977] RBP: ffff880138e04000 R08: 0000000000000017 R09: 0000000000000002
[    5.892524] R10: 0000000000099000 R11: 00000000000052d0 R12: 0000000039400200
[    5.893967] R13: ffff880138e05000 R14: 0000000000000ba0 R15: ffffc90000a4d000
[    5.895378] FS:  00007f167c9e6740(0000) GS:ffff88013fc00000(0000) knlGS:0000000000000000
[    5.896953] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    5.898143] CR2: 0000000000000000 CR3: 000000013c3ec002 CR4: 00000000001606f0
[    5.899542] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[    5.900962] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[    5.902552] Call Trace:
[    5.903267]  efi_runtime_map_copy+0x28/0x30
[    5.904956]  bzImage64_load+0x59d/0x736
[    5.906881]  ? arch_kexec_kernel_image_load+0x6d/0x70
[    5.908243]  ? __se_sys_kexec_file_load+0x24b/0x750
[    5.909352]  ? _cond_resched+0x19/0x30
[    5.910286]  ? do_syscall_64+0x65/0x180
[    5.911229]  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9
[    5.912365] Code: 90 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 
[    5.916235] RIP: memcpy_erms+0x6/0x10 RSP: ffffc9000058bd00
[    5.917507] CR2: 0000000000000000
[    5.918762] ---[ end trace 5cf4c4b3b93d7fdd ]---

Changing efi_get_runtime_map_size to return 0 in case runtime is disabled.

Also moving to check EFI_RUNTIME_SERVICES in efi_runtime_map_copy

Signed-off-by Dave Young <[email protected]>

---
 drivers/firmware/efi/runtime-map.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

drivers/firmware/efi/runtime-map.c +8 -2

@@ -138,12 +138,18 @@ add_sysfs_runtime_map_entry(struct kobje
 
 int efi_get_runtime_map_size(void)
 {
-	return efi.memmap.nr_map * efi.memmap.desc_size;
+	if (efi_enabled(EFI_RUNTIME_SERVICES))
+		return efi.memmap.nr_map * efi.memmap.desc_size;
+
+	return 0;
 }
 
 int efi_get_runtime_map_desc_size(void)
 {
-	return efi.memmap.desc_size;
+	if (efi_enabled(EFI_RUNTIME_SERVICES))
+		return efi.memmap.desc_size;
+
+	return 0;
 }
 
 int efi_runtime_map_copy(void *buf, size_t bufsz)

Mike, since we are going this way, I'm working on a kexec code cleanup, but it needs careful testing so still need some time.

Can you help test below efi fix and provide you tested-by if it works?

Mike, since we are going this way, I'm working on a kexec code cleanup, but it needs careful testing so still need some time.

Can you help test below efi fix and provide you tested-by if it works?

Quoted 5 lines · Dave Young

Mike, since we are going this way, I'm working on a kexec code cleanup, but it needs careful testing so still need some time.

Can you help test below efi fix and provide you tested-by if it works?

While it averts the efi=noruntime oops on kdump kernel load, the kernel does not boot when kdump is triggered. Bailing in setup_efi_state() in addition restores functionality.

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 <[email protected]>

---
 arch/x86/kernel/kexec-bzimage64.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

arch/x86/kernel/kexec-bzimage64.c +10 -9

@@ -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);

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.

arch/x86/kernel/kexec-bzimage64.c +11 -6

index 7326078eaa7a..e34ba2f53cfb 100644
@@ -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 */

Quoted 59 lines · Mike Galbraith

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 <[email protected]>

---
 arch/x86/kernel/kexec-bzimage64.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

arch/x86/kernel/kexec-bzimage64.c +10 -9

@@ -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);

arch/x86/kernel/kexec-bzimage64.c static void *bzImage64_load(struct kimag · line 415

Quoted 10 lines

arch/x86/kernel/kexec-bzimage64.c +1 -2

+	}
 
 	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.

arch/x86/kernel/kexec-bzimage64.c +8 -5

index 7326078eaa7a..e34ba2f53cfb 100644
@@ -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;

arch/x86/kernel/kexec-bzimage64.c +1 -0

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

Review setup_boot_parameters(struct kimage *image, struct boot_params *params, · line 258

Quoted 9 lines

(quoted diff) +2 -1

@@ -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..

Quoted 11 lines · Dave Young

Review setup_boot_parameters(struct kimage *image, struct boot_params *params, · line 258

Quoted 9 lines

(quoted diff) +2 -1

@@ -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.

Quoted 5 lines

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 <[email protected]>

---
 arch/x86/kernel/kexec-bzimage64.c |   99 +++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 54 deletions(-)

arch/x86/kernel/kexec-bzimage64.c +44 -52

@@ -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;

Quoted 1 line

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..