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

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

[Ard Biesheuvel]

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,
 		};
 
-- 
2.17.1

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

[Ingo Molnar]

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

[tip: efi/urgent] efi/x86: Fix boot regression on systems with invalid memmap entries

[tip-bot2 for Ard Biesheuvel]

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     59365cadfbcd299b8cdbe0c165faf15767c5f166
Gitweb:        https://git.kernel.org/tip/59365cadfbcd299b8cdbe0c165faf15767c5f166
Author:        Ard Biesheuvel <ardb@kernel.org>
AuthorDate:    Sun, 02 Feb 2020 00:33:04 +01:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sun, 02 Feb 2020 10:25:43 +01:00

efi/x86: Fix boot regression on systems with invalid memmap entries

In efi_clean_memmap(), we do a pass over the EFI memory map to remove
bogus entries that may be returned on certain systems.

This recent 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.

Reported-by: Jörg Otte <jrg.otte@gmail.com>
Tested-by: Jörg Otte <jrg.otte@gmail.com>
Tested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Cc: linux-efi@vger.kernel.org
Cc: jrg.otte@gmail.com
Cc: torvalds@linux-foundation.org
Cc: mingo@kernel.org
Link: https://lore.kernel.org/r/20200201233304.18322-1-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 59f7f6d..ae923ee 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,
 		};
 

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

[Ard Biesheuvel]

On Sun, 2 Feb 2020 at 10:34, Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * 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?
>

Not sure - it seems it just gets confused, given that the below

int foo(void)
{
        struct {
                int i;
                int j;
        } data = { .j = 0, .i = data.j };

        return data.i;
}

does give me

/tmp/foo.c: In function ‘foo’:
/tmp/foo.c:7:30: warning: ‘data.j’ is used uninitialized in this
function [-Wuninitialized]
  } data = { .j = 0, .i = data.j };
                          ~~~~^~
while the warnings go away when I reorder i and j in the struct definition.

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

Sure, I'll incorporate that.

> 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.
>

It's not firmware ABI, and even the memory map itself is not firmware
ABI apart from the early call to SetVirtualAddressMap where the
firmware consumes a memory map provided by the kernel.

I'll put this on my list of things to look at. FYI I am current doing
another pass of improvements aimed at unifying the boot flow between
architectures even more, and adding support for UEFI spec changes that
permit firmware implementations to omit certain runtime services. So
expect another couple of PRs over the coming six weeks or so