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 <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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
* Ard Biesheuvel <[email protected]> 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 <[email protected]>
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> 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 <[email protected]>
Tested-by: J?rg Otte <[email protected]>
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 <[email protected]>
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);
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 <[email protected]>
AuthorDate: Sun, 02 Feb 2020 00:33:04 +01:00
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Tested-by: Jörg Otte <[email protected]>
Tested-by: Dan Williams <[email protected]>
Signed-off-by: Ard Biesheuvel <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Link: https://lore.kernel.org/r/[email protected]
---
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,
};
On Sun, 2 Feb 2020 at 10:34, Ingo Molnar <[email protected]> wrote:
>
>
> * Ard Biesheuvel <[email protected]> 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 <[email protected]>
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > 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 <[email protected]>
> Tested-by: Jörg Otte <[email protected]>
>
> 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