Hello Ard,
On 6/12/2024 8:56 AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
>
> The logic in __efi_memmap_init() is shared between two different
> execution flows:
> - mapping the EFI memory map early or late into the kernel VA space, so
> that its entries can be accessed;
> - cloning the EFI memory map in order to insert new entries that are
> created as a result of creating a memory reservation
> (efi_arch_mem_reserve())
>
> In the former case, the underlying memory containing the kernel's view
> of the EFI memory map (which may be heavily modified by the kernel
> itself on x86) is not modified at all, and the only thing that changes
> is the virtual mapping of this memory, which is different between early
> and late boot.
>
> In the latter case, an entirely new allocation is created that carries a
> new, updated version of the kernel's view of the EFI memory map. When
> installing this new version, the old version will no longer be
> referenced, and if the memory was allocated by the kernel, it will leak
> unless it gets freed.
>
> The logic that implements this freeing currently lives on the code path
> that is shared between these two use cases, but it should only apply to
> the latter. So move it to the correct spot.
>
> While at it, move __efi_memmap_free() into its only caller, and drop the
> dummy definition for non-x86 architectures, as that is no longer needed.
>
> Cc: Ashish Kalra <Ashish.Kalra@xxxxxxx>
> Cc: Dave Young <dyoung@xxxxxxxxxx>
> Cc: Mike Rapoport <rppt@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks")
> Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@xxxxxxx
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> v3:
> - don't move __efi_memmap_free() into what turned out not to be its only
> caller
> - drop another CPP #define related to the dummy definition
>
> v2:
> - free old memory map only after installing the new one succeeded
> - move __efi_memmap_free() into its only caller
> - drop obsolete dummy declaration from generic code
>
> arch/x86/include/asm/efi.h | 1 -
> arch/x86/platform/efi/memmap.c | 12 +++++++++++-
> drivers/firmware/efi/memmap.c | 9 ---------
> 3 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 1dc600fa3ba5..481096177500 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -401,7 +401,6 @@ extern int __init efi_memmap_alloc(unsigned int num_entries,
> struct efi_memory_map_data *data);
> extern void __efi_memmap_free(u64 phys, unsigned long size,
> unsigned long flags);
> -#define __efi_memmap_free __efi_memmap_free
>
> extern int __init efi_memmap_install(struct efi_memory_map_data *data);
> extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
> diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c
> index 4ef20b49eb5e..6ed1935504b9 100644
> --- a/arch/x86/platform/efi/memmap.c
> +++ b/arch/x86/platform/efi/memmap.c
> @@ -92,12 +92,22 @@ int __init efi_memmap_alloc(unsigned int num_entries,
> */
> int __init efi_memmap_install(struct efi_memory_map_data *data)
> {
> + unsigned long size = efi.memmap.desc_size * efi.memmap.nr_map;
> + unsigned long flags = efi.memmap.flags;
> + u64 phys = efi.memmap.phys_map;
> + int ret;
> +
> efi_memmap_unmap();
>
> if (efi_enabled(EFI_PARAVIRT))
> return 0;
>
> - return __efi_memmap_init(data);
> + ret = __efi_memmap_init(data);
> + if (ret)
> + return ret;
> +
> + __efi_memmap_free(phys, size, flags);
> + return 0;
> }
>
> /**
> diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c
> index 3365944f7965..34109fd86c55 100644
> --- a/drivers/firmware/efi/memmap.c
> +++ b/drivers/firmware/efi/memmap.c
> @@ -15,10 +15,6 @@
> #include <asm/early_ioremap.h>
> #include <asm/efi.h>
>
> -#ifndef __efi_memmap_free
> -#define __efi_memmap_free(phys, size, flags) do { } while (0)
> -#endif
> -
> /**
> * __efi_memmap_init - Common code for mapping the EFI memory map
> * @data: EFI memory map data
> @@ -51,11 +47,6 @@ int __init __efi_memmap_init(struct efi_memory_map_data *data)
> return -ENOMEM;
> }
>
> - if (efi.memmap.flags & (EFI_MEMMAP_MEMBLOCK | EFI_MEMMAP_SLAB))
> - __efi_memmap_free(efi.memmap.phys_map,
> - efi.memmap.desc_size * efi.memmap.nr_map,
> - efi.memmap.flags);
> -
> map.phys_map = data->phys_map;
> map.nr_map = data->size / data->desc_size;
> map.map_end = map.map + data->size;
Tested this patch with SNP guest kexec and i do not observe EFI memory map corruption with SNP guest kexec with this patch applied.
I have another question related to the discussion on the following email thread:
Re: [PATCH v7 1/3] efi/x86: Fix EFI memory map corruption with kexec - Dave Young (kernel.org) <https://lore.kernel.org/all/CALu+AoQ2jNjb+5MZfFtpT_Y=2gJRWwqEeWTpQkwNt0-U5fpO_w@xxxxxxxxxxxxxx/><https://lore.kernel.org/all/CALu+AoQ2jNjb+5MZfFtpT_Y=2gJRWwqEeWTpQkwNt0-U5fpO_w@xxxxxxxxxxxxxx/>
Do we still want to create another patch for skipping efi_arch_mem_reserve() when the EFI_MEMORY_RUNTIME bit was set already ?
Tested-by: Ashish Kalra <Ashish.Kalra@xxxxxxx>
