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

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]

Date:   Sun, 2 Feb 2020 10:34:42 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     linux-efi@...r.kernel.org, x86@...nel.org,
        dan.j.williams@...el.com, jrg.otte@...il.com,
        torvalds@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] efi/x86: fix boot regression on systems with invalid
 memmap entries


* Ard Biesheuvel <ardb@...nel.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@...el.com>
> Signed-off-by: Ard Biesheuvel <ardb@...nel.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@...il.com>
    Tested-by: Jörg Otte <jrg.otte@...il.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@...nel.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);