Received-SPF: pass (google.com: domain of linux-kernel+bounces-214046-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240612135638.298882-2-ardb+git@google.com> <16131a10-b473-41cc-a96e-d71a4d930353@amd.com>
In-Reply-To: <16131a10-b473-41cc-a96e-d71a4d930353@amd.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Fri, 14 Jun 2024 00:02:20 +0200
Message-ID: <CAMj1kXExC47bMuRPatthoJQqAy=6fbN=5KZfEq05P+2dbLRcUA@mail.gmail.com>
Subject: Re: [PATCH v3] x86/efi: Free EFI memory map only when installing a
 new one.
To: "Kalra, Ashish" <ashish.kalra@amd.com>
Cc: Ard Biesheuvel <ardb+git@google.com>, linux-efi@vger.kernel.org, 
	linux-kernel@vger.kernel.org, x86@kernel.org, Dave Young <dyoung@redhat.com>, 
	Mike Rapoport <rppt@kernel.org>, Borislav Petkov <bp@alien8.de>, Dan Williams <dan.j.williams@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

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 th=
e
> > 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 onl=
y
> >   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/mem=
map.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_entrie=
s,
> >   */
> >  int __init efi_memmap_install(struct efi_memory_map_data *data)
> >  {
> > +     unsigned long size =3D efi.memmap.desc_size * efi.memmap.nr_map;
> > +     unsigned long flags =3D efi.memmap.flags;
> > +     u64 phys =3D efi.memmap.phys_map;
> > +     int ret;
> > +
> >       efi_memmap_unmap();
> >
> >       if (efi_enabled(EFI_PARAVIRT))
> >               return 0;
> >
> > -     return __efi_memmap_init(data);
> > +     ret =3D __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/memma=
p.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_d=
ata *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_ma=
p,
> > -                               efi.memmap.flags);
> > -
> >       map.phys_map =3D data->phys_map;
> >       map.nr_map =3D data->size / data->desc_size;
> >       map.map_end =3D map.map + data->size;
>
> Tested this patch with SNP guest kexec and i do not observe EFI memory ma=
p 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 - Da=
ve Young (kernel.org) <https://lore.kernel.org/all/CALu+AoQ2jNjb+5MZfFtpT_Y=
=3D2gJRWwqEeWTpQkwNt0-U5fpO_w@mail.gmail.com/><https://lore.kernel.org/all/=
CALu+AoQ2jNjb+5MZfFtpT_Y=3D2gJRWwqEeWTpQkwNt0-U5fpO_w@mail.gmail.com/>
>
> Do we still want to create another patch for skipping efi_arch_mem_reserv=
e() when the EFI_MEMORY_RUNTIME bit was set already ?
>

If it is not needed, let's not bother.