linux-kernel - Re: [TEST PATCH RFC] Revert the EFI leak fixes for now (was: Re: EFI boot crash regression (was: Re: 5.6-### doesn't boot))

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPcyv4iH6BA4Oo7ZedWMG_28cp1KauQET58qOpR7012aaHWzbQ@mail.gmail.com>
Date:   Sat, 1 Feb 2020 08:23:42 -0800
From:   Dan Williams <dan.j.williams@...el.com>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     Jörg Otte <jrg.otte@...il.com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>
Subject: Re: [TEST PATCH RFC] Revert the EFI leak fixes for now (was: Re: EFI
 boot crash regression (was: Re: 5.6-### doesn't boot))

On Sat, Feb 1, 2020 at 1:45 AM Ingo Molnar <mingo@...nel.org> wrote:
>
>
>
> Jörg Otte wrote:
>
> > It's bisected.
> > The first bad commit is :
> > 1db91035d01aa8bfa2350c00ccb63d629b4041ad
> > efi: Add tracking for dynamically allocated memmaps
>
> > Unfortunately I can not revert because of compile errors!
> >
> > In file included from /media/jojo/deftoshiba/kernel/linux/init/main.c:48:
> > /media/jojo/deftoshiba/kernel/linux/include/linux/efi.h:975:1: error:
> > version control conflict marker in file
> > <<<<<<< HEAD
>
> So 1db91035d0 doesn't revert cleanly, because 484a418d0754 depends on it,
> plus there a third commit (f0ef6523475f) that has a semantic dependency
> on 1db91035d01a.
>
> But you can revert them all, if done in reverse chronological order:
>
>   git revert f0ef6523475f # ("efi: Fix efi_memmap_alloc() leaks")
>   git revert 484a418d0754 # ("efi: Fix handling of multiple efi_fake_mem= entries")
>   git revert 1db91035d01a # ("efi: Add tracking for dynamically allocated memmaps")
>
> You can paste those three lines into a shell as-is, or you can apply the
> patch below which is a combination of these three reverts.
>
> Jörg, can you confirm that doing these reverts fixes booting on your
> system? If it does then a full dmesg from that kernel would be useful.
>
> FWIW I reviewed the bisected commit and didn't find the bug but I also
> couldn't convince myself it's a 1:1 identity transformation and cleanup
> of the existing logic.
>
> The size arithmethics transformation looks correct at first sight, but
> the data->flags handling in particular looks rather interwoven.

Agreed, but the only flags change that I couldn't convince myself was
correct is this:

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 59f7f6d60cf6..314b36ac2a08 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -309,7 +309,7 @@ static void __init efi_clean_memmap(void)
                        .desc_version = efi.memmap.desc_version,
                        .desc_size = efi.memmap.desc_size,
                        .size = data.desc_size * (efi.memmap.nr_map -
n_removal),
-                       .flags = 0,
+                       .flags = efi.memmap.flags & EFI_MEMMAP_LATE,
                };

                pr_warn("Removing %d invalid memory map entries.\n", n_removal);

...but efi_clean_memmap() should "early".