On Mon, 10 Jun 2024 at 22:09, Ard Biesheuvel <ardb+git@xxxxxxxxxx> 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. > > Cc: Ashish Kalra <Ashish.Kalra@xxxxxxx> > Cc: Dave Young <dyoung@xxxxxxxxxx> > Cc: Mike Rapoport <rppt@xxxxxxxxxx> > Cc: Borislav Petkov <bp@xxxxxxxxx> > Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@xxxxxxx > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > --- > arch/x86/platform/efi/memmap.c | 5 +++++ > drivers/firmware/efi/memmap.c | 5 ----- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/x86/platform/efi/memmap.c b/arch/x86/platform/efi/memmap.c > index 4ef20b49eb5e..4990244e5168 100644 > --- a/arch/x86/platform/efi/memmap.c > +++ b/arch/x86/platform/efi/memmap.c > @@ -97,6 +97,11 @@ int __init efi_memmap_install(struct efi_memory_map_data *data) > if (efi_enabled(EFI_PARAVIRT)) > return 0; > > + 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); > + > return __efi_memmap_init(data); Nice fix! but would it be better to save the old addr/size/flag and free them only when __efi_memmap_init succeed? > } > > diff --git a/drivers/firmware/efi/memmap.c b/drivers/firmware/efi/memmap.c > index 3365944f7965..3759e95a7407 100644 > --- a/drivers/firmware/efi/memmap.c > +++ b/drivers/firmware/efi/memmap.c > @@ -51,11 +51,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; > -- > 2.45.2.505.gda0bf45e8d-goog >
