Re: [PATCH] efi/x86: fix boot regression on systems with invalid memmap entries

From: Ingo Molnar
Date: Sun Feb 02 2020 - 04:34:49 EST



* Ard Biesheuvel <ardb@xxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
Tested-by: Jörg Otte <jrg.otte@xxxxxxxxx>

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

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