* 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?
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@xxxxxxxxxx>
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);
