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

[Ingo Molnar <mingo@kernel.org>]

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