Ard Biesheuvel [off-list ref] writes:

On 5 January 2017 at 12:51, Nicolai Stange [off-list ref] wrote:

quoted

Before invoking the arch specific handler, efi_mem_reserve() reserves
the given memory region through memblock.

efi_mem_reserve() can get called after mm_init() though -- through
efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
not be used anymore.

Let efi_mem_reserve() check whether memblock is dead and not do the
reservation if so. Emit a warning from the generic efi_arch mem_reserve()
in this case: if the architecture doesn't provide any other means of
registering the region as reserved, the operation would be a nop.

Fixes: 4bc9f92e64c8 ("x86/efi-bgrt: Use efi_mem_reserve() to avoid copying image data")
Signed-off-by: Nicolai Stange <redacted>
---
Applicable to next-20170105.
No changes to v2.
Boot-tested on x86_64.

 drivers/firmware/efi/efi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 92914801e388..158a8df2f4af 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c

@@ -403,7 +403,10 @@ u64 __init efi_mem_desc_end(efi_memory_desc_t *md)
        return end;
 }

-void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
+void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size)
+{
+       WARN(slab_is_available(), "efi_mem_reserve() has no effect");
+}

 /**
  * efi_mem_reserve - Reserve an EFI memory region

@@ -419,7 +422,7 @@ void __init __weak efi_arch_mem_reserve(phys_addr_t addr, u64 size) {}
  */
 void __init efi_mem_reserve(phys_addr_t addr, u64 size)
 {
-       if (!memblock_is_region_reserved(addr, size))
+       if (!slab_is_available() && !memblock_is_region_reserved(addr, size))
                memblock_reserve(addr, size);

More context:

            /*
             * Some architectures (x86) reserve all boot services ranges
             * until efi_free_boot_services() because of buggy firmware
             * implementations. This means the above memblock_reserve() is
             * superfluous on x86 and instead what it needs to do is
             * ensure the @start, @size is not freed.
             */
            efi_arch_mem_reserve(addr, size);
    }

I share Dave's concern: on x86, this will silently ignore the
reservation if slab_is_available() returns true,

AFAICS, x86 has got an efi_arch_mem_reserve() which doesn't ignore the
reservation at any stage.

The default implementation of efi_arch_mem_reserve() used on ARM is
empty though ... 

so we should at least warn here.

... and this patch adds a WARN() to the empty stub.

I don't think this patch solves any known issues, so I'd
rather defer this for now, and pick up the discussion when Matt is
back,

I'm fine with either way and yes, no splat has been observed in the
wild.

Just to make it explicit: the issue addressed here is a potential
use-after-free (both, read and write) on memblock.reserved.regions in
case of CONFIG_ARCH_DISCARD_MEMBLOCK=y. It would certainly make sense to
clarify the commit description in the next iteration...

Thanks,

Nicolai