Messages in this thread Patch in this message | | | Date | Sun, 2 Feb 2020 10:34:42 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH] efi/x86: fix boot regression on systems with invalid memmap entries |
| |
* Ard Biesheuvel <ardb@kernel.org> wrote:
> In efi_clean_memmap(), we do a pass over the EFI memory map to remove > bogus entries that may be returned on certain systems. > > Commit 1db91035d01aa8bf ("efi: Add tracking for dynamically allocated > memmaps") refactored this code to pass the input to efi_memmap_install() > via a temporary struct on the stack, which is populated using an > initializer which inadvertently defines the value of its size field > in terms of its desc_size field, which value cannot be relied upon yet > in the initializer itself. > > Fix this by using efi.memmap.desc_size instead, which is where we get > the value for desc_size from in the first place. > > Tested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/platform/efi/efi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c > index 59f7f6d60cf6..ae923ee8e2b4 100644 > --- a/arch/x86/platform/efi/efi.c > +++ b/arch/x86/platform/efi/efi.c > @@ -308,7 +308,7 @@ static void __init efi_clean_memmap(void) > .phys_map = efi.memmap.phys_map, > .desc_version = efi.memmap.desc_version, > .desc_size = efi.memmap.desc_size, > - .size = data.desc_size * (efi.memmap.nr_map - n_removal), > + .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal), > .flags = 0, > };
Applied, and I also added:
Reported-by: Jörg Otte <jrg.otte@gmail.com> Tested-by: Jörg Otte <jrg.otte@gmail.com>
I presumptively added the Jörg's Tested-by tag: won't send the commit to Linus if he still has trouble booting the laptop.
I'm still amazed GCC doesn't warn about this pattern - why?
BTW., could we please also organize such assignments vertically as well:
.phys_map = efi.memmap.phys_map, .desc_version = efi.memmap.desc_version, .desc_size = efi.memmap.desc_size, .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal), .flags = 0,
(See the patch below.)
Had we done that earlier the weird pattern would have stuck out a lot more:
.phys_map = efi.memmap.phys_map, .desc_version = efi.memmap.desc_version, .desc_size = efi.memmap.desc_size, .size = data.desc_size * (efi.memmap.nr_map - n_removal), .flags = 0,
BTW., is there a reason "struct efi_memory_map" doesn't embedd a "struct efi_memory_map_data"? Or is efi_memory_map firmware ABI?
If they shared the structure then copying would be easier.
Thanks,
Ingo
Signed-off-by: Ingo Molnar <mingo@kernel.org>
arch/x86/platform/efi/efi.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c index ae923ee8e2b4..293c47f9cb39 100644 --- a/arch/x86/platform/efi/efi.c +++ b/arch/x86/platform/efi/efi.c @@ -305,11 +305,11 @@ static void __init efi_clean_memmap(void) if (n_removal > 0) { struct efi_memory_map_data data = { - .phys_map = efi.memmap.phys_map, - .desc_version = efi.memmap.desc_version, - .desc_size = efi.memmap.desc_size, - .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal), - .flags = 0, + .phys_map = efi.memmap.phys_map, + .desc_version = efi.memmap.desc_version, + .desc_size = efi.memmap.desc_size, + .size = efi.memmap.desc_size * (efi.memmap.nr_map - n_removal), + .flags = 0, }; pr_warn("Removing %d invalid memory map entries.\n", n_removal);
| |