Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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 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.ca...@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 > > [5.879868] PGD 80013c1f1067 P4D 80013c1f1067 PUD 13aea7067 PMD 0 > [5.881225] Oops: [#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:c958bd00 EFLAGS: 00010246 > [5.887899] RAX: 880138e050b0 RBX: 000980b0 RCX: > 0ba0 > [5.889304] RDX: 0ba0 RSI: RDI: > 880138e050b0 > [5.890977] RBP: 880138e04000 R08: 0017 R09: > 0002 > [5.892524] R10: 00099000 R11: 52d0 R12: > 39400200 > [5.893967] R13: 880138e05000 R14: 0ba0 R15: > c9a4d000 > [5.895378] FS: 7f167c9e6740() GS:88013fc0() > knlGS: > [5.896953] CS: 0010 DS: ES: CR0: 80050033 > [5.898143] CR2: CR3: 00013c3ec002 CR4: > 001606f0 > [5.899542] DR0: DR1: DR2: > > [5.900962] DR3: DR6: fffe0ff0 DR7: > 0400 > [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 > 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: c958bd00 > [5.917507] CR2: > [5.918762] ---[ end trace 5cf4c4b3b93d7fdd ]--- > > Changing
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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 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.ca...@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 > > [5.879868] PGD 80013c1f1067 P4D 80013c1f1067 PUD 13aea7067 PMD 0 > [5.881225] Oops: [#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:c958bd00 EFLAGS: 00010246 > [5.887899] RAX: 880138e050b0 RBX: 000980b0 RCX: > 0ba0 > [5.889304] RDX: 0ba0 RSI: RDI: > 880138e050b0 > [5.890977] RBP: 880138e04000 R08: 0017 R09: > 0002 > [5.892524] R10: 00099000 R11: 52d0 R12: > 39400200 > [5.893967] R13: 880138e05000 R14: 0ba0 R15: > c9a4d000 > [5.895378] FS: 7f167c9e6740() GS:88013fc0() > knlGS: > [5.896953] CS: 0010 DS: ES: CR0: 80050033 > [5.898143] CR2: CR3: 00013c3ec002 CR4: > 001606f0 > [5.899542] DR0: DR1: DR2: > > [5.900962] DR3: DR6: fffe0ff0 DR7: > 0400 > [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 > 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: c958bd00 > [5.917507] CR2: > [5.918762] ---[ end trace 5cf4c4b3b93d7fdd ]--- > > Changing
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
> ---
> 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 = >efi_info;
>
> - if (!efi_map_sz)
> - return -EINVAL;
> -
> efi_runtime_map_copy(efi_map, efi_map_sz);
>
> ei->efi_memmap = efi_map_phys_addr & 0x;
> 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 = _params.efi_info;
> struct efi_info *ei =
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
> ---
> 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 = >efi_info;
>
> - if (!efi_map_sz)
> - return -EINVAL;
> -
> efi_runtime_map_copy(efi_map, efi_map_sz);
>
> ei->efi_memmap = efi_map_phys_addr & 0x;
> 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 = _params.efi_info;
> struct efi_info *ei =
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
---
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 = >efi_info;
- if (!efi_map_sz)
- return -EINVAL;
-
efi_runtime_map_copy(efi_map, efi_map_sz);
ei->efi_memmap = efi_map_phys_addr & 0x;
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 = _params.efi_info;
struct efi_info *ei = >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= 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 =
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
---
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 = >efi_info;
- if (!efi_map_sz)
- return -EINVAL;
-
efi_runtime_map_copy(efi_map, efi_map_sz);
ei->efi_memmap = efi_map_phys_addr & 0x;
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 = _params.efi_info;
struct efi_info *ei = >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= 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 =
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
> > ---
> > 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 = >efi_info;
> >
> > - if (!efi_map_sz)
> > - return 0;
> > -
> > efi_runtime_map_copy(efi_map, efi_map_sz);
> >
> > ei->efi_memmap = efi_map_phys_addr & 0x;
> > @@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
> > * acpi_rsdp= 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 = >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 = _params.efi_info;
> struct efi_info *ei = >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= on kernel command line to make second kernel boot
>* without efi.
>*/
> - if
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
> > ---
> > 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 = >efi_info;
> >
> > - if (!efi_map_sz)
> > - return 0;
> > -
> > efi_runtime_map_copy(efi_map, efi_map_sz);
> >
> > ei->efi_memmap = efi_map_phys_addr & 0x;
> > @@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
> > * acpi_rsdp= 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 = >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 = _params.efi_info;
> struct efi_info *ei = >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= on kernel command line to make second kernel boot
>* without efi.
>*/
> - if
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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 = >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 = _params.efi_info;
> struct efi_info *ei = >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= 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
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 = >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 = _params.efi_info;
> struct efi_info *ei = >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= 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
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
> ---
> 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 = >efi_info;
>
> - if (!efi_map_sz)
> - return 0;
> -
> efi_runtime_map_copy(efi_map, efi_map_sz);
>
> ei->efi_memmap = efi_map_phys_addr & 0x;
> @@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
>* acpi_rsdp= 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 = >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 = _params.efi_info;
struct efi_info *ei = >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= 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
Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference
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
> ---
> 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 = >efi_info;
>
> - if (!efi_map_sz)
> - return 0;
> -
> efi_runtime_map_copy(efi_map, efi_map_sz);
>
> ei->efi_memmap = efi_map_phys_addr & 0x;
> @@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
>* acpi_rsdp= 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 = >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 = _params.efi_info;
struct efi_info *ei = >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= 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
[PATCH] 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
---
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 = >efi_info;
- if (!efi_map_sz)
- return 0;
-
efi_runtime_map_copy(efi_map, efi_map_sz);
ei->efi_memmap = efi_map_phys_addr & 0x;
@@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
* acpi_rsdp= 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;
[PATCH] 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
---
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 = >efi_info;
- if (!efi_map_sz)
- return 0;
-
efi_runtime_map_copy(efi_map, efi_map_sz);
ei->efi_memmap = efi_map_phys_addr & 0x;
@@ -176,7 +173,7 @@ setup_efi_state(struct boot_params *para
* acpi_rsdp= 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;
