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@xxxxxxxxxx> wrote:
>
>
> * 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?
>

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