| Message ID | 20240612135638.298882-2-ardb+git@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] x86/efi: Free EFI memory map only when installing a new one. | expand |
On Wed, 12 Jun 2024 at 15:56, Ard Biesheuvel <ardb+git@google.com> wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > 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@amd.com> > Cc: Dave Young <dyoung@redhat.com> > Cc: Mike Rapoport <rppt@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dan Williams <dan.j.williams@intel.com> > Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks") > Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@amd.com > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > 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(-) > Given that this fixes an actual use-after-free bug that is not specific to TDX/SEV or kexec, I'm inclined to queue this up as a fix. @Boris: Mind if I take this as an EFI fix? Otherwise, can you queue this up? Thanks.
Hello Ard, On 6/12/2024 8:56 AM, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > 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@amd.com> > Cc: Dave Young <dyoung@redhat.com> > Cc: Mike Rapoport <rppt@kernel.org> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Dan Williams <dan.j.williams@intel.com> > Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks") > Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@amd.com > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > 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@mail.gmail.com/><https://lore.kernel.org/all/CALu+AoQ2jNjb+5MZfFtpT_Y=2gJRWwqEeWTpQkwNt0-U5fpO_w@mail.gmail.com/> 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@amd.com>
On Thu, 13 Jun 2024 at 22:05, Kalra, Ashish <ashish.kalra@amd.com> wrote: > > Hello Ard, > > On 6/12/2024 8:56 AM, Ard Biesheuvel wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > > > 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@amd.com> > > Cc: Dave Young <dyoung@redhat.com> > > Cc: Mike Rapoport <rppt@kernel.org> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks") > > Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@amd.com > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > 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. > > Tested-by: Ashish Kalra <Ashish.Kalra@amd.com> > Thanks! > 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@mail.gmail.com/><https://lore.kernel.org/all/CALu+AoQ2jNjb+5MZfFtpT_Y=2gJRWwqEeWTpQkwNt0-U5fpO_w@mail.gmail.com/> > > Do we still want to create another patch for skipping efi_arch_mem_reserve() when the EFI_MEMORY_RUNTIME bit was set already ? > If it is not needed, let's not bother.
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;