From: Ard Biesheuvel <[email protected]>
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 <[email protected]>
Cc: Dave Young <[email protected]>
Cc: Mike Rapoport <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dan Williams <[email protected]>
Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks")
Link: https://lore.kernel.org/all/[email protected]
Signed-off-by: Ard Biesheuvel <[email protected]>
---
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;
--
2.45.2.505.gda0bf45e8d-goog
On Wed, 12 Jun 2024 at 15:56, Ard Biesheuvel <[email protected]> wrote:
>
> From: Ard Biesheuvel <[email protected]>
>
> 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 <[email protected]>
> Cc: Dave Young <[email protected]>
> Cc: Mike Rapoport <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Dan Williams <[email protected]>
> Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks")
> Link: https://lore.kernel.org/all/[email protected]
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> 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.
On Thu, Jun 13, 2024 at 10:28:05AM +0200, Ard Biesheuvel wrote:
> 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.
Sure, go ahead:
$ git log master..tip/master arch/x86/platform/efi/memmap.c
$
Should be fine, conflict-wise.
:-)
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Hello Ard,
On 6/12/2024 8:56 AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <[email protected]>
>
> 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 <[email protected]>
> Cc: Dave Young <[email protected]>
> Cc: Mike Rapoport <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Dan Williams <[email protected]>
> Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks")
> Link: https://lore.kernel.org/all/[email protected]
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
> 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 <[email protected]>
On Thu, 13 Jun 2024 at 22:05, Kalra, Ashish <[email protected]> wrote:
>
> Hello Ard,
>
> On 6/12/2024 8:56 AM, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Dave Young <[email protected]>
> > Cc: Mike Rapoport <[email protected]>
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Dan Williams <[email protected]>
> > Fixes: f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks")
> > Link: https://lore.kernel.org/all/[email protected]
> > Signed-off-by: Ard Biesheuvel <[email protected]>
> > ---
> > 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 <[email protected]>
>
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.