linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Ingo Molnar <mingo@kernel.org>
Cc: "Ard Biesheuvel" <ardb@kernel.org>,
	linux-efi <linux-efi@vger.kernel.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	"Williams, Dan J" <dan.j.williams@intel.com>,
	"Jörg Otte" <jrg.otte@gmail.com>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] efi/x86: fix boot regression on systems with invalid memmap entries
Date: Sun, 2 Feb 2020 10:50:39 +0100	[thread overview]
Message-ID: <CAKv+Gu9V7hHFaFZmz=U_PP82wxP+J2rqVUasuOpvm-Jjg96OdA@mail.gmail.com> (raw)
In-Reply-To: <20200202093442.GB72728@gmail.com>

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

  reply	other threads:[~2020-02-02  9:50 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-01 23:33 [PATCH] efi/x86: fix boot regression on systems with invalid memmap entries Ard Biesheuvel
2020-02-02  9:34 ` Ingo Molnar
2020-02-02  9:50   ` Ard Biesheuvel [this message]
2020-02-02  9:39 ` [tip: efi/urgent] efi/x86: Fix " tip-bot2 for Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKv+Gu9V7hHFaFZmz=U_PP82wxP+J2rqVUasuOpvm-Jjg96OdA@mail.gmail.com' \
    --to=ard.biesheuvel@linaro.org \
    --cc=ardb@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=jrg.otte@gmail.com \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).