diff mbox series

x86/efi: Free EFI memory map only when installing a new one.

Message ID 20240610140932.2489527-2-ardb+git@google.com
State New
Headers show
Series x86/efi: Free EFI memory map only when installing a new one. | expand

Commit Message

Ard Biesheuvel June 10, 2024, 2:09 p.m. UTC 
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.

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>
Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@amd.com
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/memmap.c | 5 +++++
 drivers/firmware/efi/memmap.c  | 5 -----
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Borislav Petkov June 10, 2024, 2:11 p.m. UTC | #1 
+ Dan because of

f0ef6523475f ("efi: Fix efi_memmap_alloc() leaks")

Thanks Ard.

On Mon, Jun 10, 2024 at 04:09:33PM +0200, 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.
> 
> 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>
> Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@amd.com
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  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);
>  }
>  
> 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
>
Dave Young June 12, 2024, 10:36 a.m. UTC | #2 
On Mon, 10 Jun 2024 at 22:09, 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.
>
> 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>
> Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@amd.com
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  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
>
Ard Biesheuvel June 12, 2024, 12:29 p.m. UTC | #3 
On Wed, 12 Jun 2024 at 12:36, Dave Young <dyoung@redhat.com> wrote:
>
> On Mon, 10 Jun 2024 at 22:09, 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.
> >
> > 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>
> > Link: https://lore.kernel.org/all/36ad5079-4326-45ed-85f6-928ff76483d3@amd.com
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  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?
>

Yeah. I guess the same applies to the original code, but now is as
good a time as ever to fix that as well.
diff mbox series

Patch

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);
 }
 
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;