linux-kernel - Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into boot mem areas

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]

Date:   Wed, 18 Jan 2017 19:01:05 +0800
From:   Dave Young <dyoung@...hat.com>
To:     Nicolai Stange <nicstange@...il.com>
Cc:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "hpa@...or.com" <hpa@...or.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        Bhupesh Sharma <bhsharma@...hat.com>
Subject: Re: [PATCH V2 4/4] efi/x86: make efi_memmap_reserve only insert into
 boot mem areas

On 01/17/17 at 08:48pm, Nicolai Stange wrote:
> On Tue, Jan 17 2017, Ard Biesheuvel wrote:
> 
> > On 16 January 2017 at 02:45, Dave Young <dyoung@...hat.com> wrote:
> >> efi_mem_reserve cares only about boot services regions, for making sure
> >> later efi_free_boot_services does not free areas which are still useful,
> >> such as bgrt image buffer.
> >>
> >> So add a new argument to efi_memmap_insert for this purpose.
> >>
> >
> > So what happens is we try to efi_mem_reserve() a regions that is not
> > bootservices code or data?
> > We shouldn't simply ignore it, because it is a serious condition.
> 
> The efi_mem_desc_lookup() call in efi_arch_mem_reserve() wouldn't return
> anything and the latter would
> 
>   pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
> 
> then.
> 
> This is so because efi_mem_desc_lookup() searches only for regions that
> either
> - are of type EFI_BOOT_SERVICES_DATA or EFI_RUNTIME_SERVICES_DATA
> - or which have EFI_MEMORY_RUNTIME set already:
> 
> 	if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
> 	    md->type != EFI_BOOT_SERVICES_DATA &&
> 	    md->type != EFI_RUNTIME_SERVICES_DATA) {
> 		continue;
> 	}
> 
> For EFI_RUNTIME_SERVICES_DATA and EFI_MEMORY_RUNTIME,
> efi_arch_mem_reserve() would be a nop.
> 
> So we're fine here? Do you want to have a more descriptive error message
> than "Failed to lookup EFI memory descriptor"?
> 
> 
> For the other checks you suggested in that other thread, i.e. for the
> post-slab_is_available() condition and so: let me wait until Dave's
> series has stabilized (or even picked) and I'll submit patches for
> what remains to be sanity checked then.
> 
> Also, since Dave eliminated the need for late efi_mem_reserve()'s,
> my 20b1e22d01a4 ("x86/efi: Don't allocate memmap through memblock after
> mm_init()") should certainly get reverted at some point.

While testing my patches with latest edk2, I found another thing to be
fixed, I will repost bgrt patch according to Ard's comment tomorrow,
maybe with below patch as another fix to the memblock_alloc late
callback.

---
 arch/x86/platform/efi/quirks.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-x86.orig/arch/x86/platform/efi/quirks.c
+++ linux-x86/arch/x86/platform/efi/quirks.c
@@ -355,7 +355,7 @@ void __init efi_free_boot_services(void)
 	}
 
 	new_size = efi.memmap.desc_size * num_entries;
-	new_phys = kzalloc(new_size, GFP_KERNEL);
+	new_phys = efi_memmap_alloc(num_entries);
 	if (!new_phys) {
 		pr_err("Failed to allocate new EFI memmap\n");
 		return;

> 
> 
> Thanks,
> 
> Nicolai