Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference

[Mike Galbraith]

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