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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1533791114.5087.30.camel@gmx.de>
Date:   Thu, 09 Aug 2018 07:05:14 +0200
From:   Mike Galbraith <efault@....de>
To:     Dave Young <dyoung@...hat.com>
Cc:     Baoquan He <bhe@...hat.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        lkml <linux-kernel@...r.kernel.org>, kexec@...ts.infradead.org
Subject: Re: [PATCH] x86, kdump: Fix efi=noruntime NULL pointer dereference

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