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

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

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

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

[Dave Young]

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

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

[Mike Galbraith]

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

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

[Mike Galbraith]

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

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

[Dave Young]

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

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

[Ard Biesheuvel]

On 9 August 2018 at 11:13, Dave Young <dyoung@redhat.com> wrote:
> 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.
>

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.

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

[Dave Young]

On 08/21/18 at 03:39pm, Ard Biesheuvel wrote:
> On 9 August 2018 at 11:13, Dave Young <dyoung@redhat.com> wrote:
> > 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.
> >
> 
> 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/1533737025.4936.3.camel@gmx.de

> 
> 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 <dyoung@redhat.com>
---
 drivers/firmware/efi/runtime-map.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

--- linux-x86.orig/drivers/firmware/efi/runtime-map.c
+++ linux-x86/drivers/firmware/efi/runtime-map.c
@@ -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)
@@ -163,7 +169,7 @@ int __init efi_runtime_map_init(struct k
 	struct efi_runtime_map_entry *entry;
 	efi_memory_desc_t *md;
 
-	if (!efi_enabled(EFI_MEMMAP))
+	if (!efi_enabled(EFI_RUNTIME_SERVICES))
 		return 0;
 
 	map_entries = kcalloc(efi.memmap.nr_map, sizeof(entry), GFP_KERNEL);

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

[Dave Young]

On 08/22/18 at 06:23pm, Dave Young wrote:
> On 08/21/18 at 03:39pm, Ard Biesheuvel wrote:
> > On 9 August 2018 at 11:13, Dave Young <dyoung@redhat.com> wrote:
> > > 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.
> > >
> > 
> > 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/1533737025.4936.3.camel@gmx.de
> 
> > 
> > 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

Forgot to add Mike's reported-by tag..

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?

> ---------------------------------------------------------------------------
> 
> 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 <dyoung@redhat.com>
> ---
>  drivers/firmware/efi/runtime-map.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> --- linux-x86.orig/drivers/firmware/efi/runtime-map.c
> +++ linux-x86/drivers/firmware/efi/runtime-map.c
> @@ -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)
> @@ -163,7 +169,7 @@ int __init efi_runtime_map_init(struct k
>  	struct efi_runtime_map_entry *entry;
>  	efi_memory_desc_t *md;
>  
> -	if (!efi_enabled(EFI_MEMMAP))
> +	if (!efi_enabled(EFI_RUNTIME_SERVICES))
>  		return 0;
>  
>  	map_entries = kcalloc(efi.memmap.nr_map, sizeof(entry), GFP_KERNEL);
> 

Thanks
Dave

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

[Mike Galbraith]

On Thu, 2018-08-23 at 11:57 +0800, Dave Young wrote:
> 
> 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?

Sure.  I'm buried to the eyebrows atm (why no patchlet has appeared in
your inbox), but I'll get to it.

	-Mike

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

[Mike Galbraith]

On Thu, 2018-08-23 at 11:57 +0800, Dave Young wrote:
> 
> 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.

	-Mike

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

[Dave Young]

On 08/24/18 at 06:48am, Mike Galbraith wrote:
> On Thu, 2018-08-23 at 11:57 +0800, Dave Young wrote:
> > 
> > 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.

Yes, that is expected.  Kexec code need fix and cleanup in other
patches.

Thanks
Dave

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

[Dave Young]

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 */

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

[Mike Galbraith]

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 */

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

[Dave Young]

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 */

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

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

[Dave Young]

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

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

[Mike Galbraith]

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