Received-SPF: pass (google.com: domain of linux-kernel+bounces-211485-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: <20240610140932.2489527-2-ardb+git@google.com> <CALu+AoREA7f=UBrmbvH92nDp66LyB99QEu84e_8HSurnjLqdUw@mail.gmail.com>
In-Reply-To: <CALu+AoREA7f=UBrmbvH92nDp66LyB99QEu84e_8HSurnjLqdUw@mail.gmail.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Wed, 12 Jun 2024 14:29:35 +0200
Message-ID: <CAMj1kXFxEwoSjHHt+1CFqMHwQyC6UVbt=5AqFvE1S5B46AbnOA@mail.gmail.com>
Subject: Re: [PATCH] x86/efi: Free EFI memory map only when installing a new one.
To: Dave Young <dyoung@redhat.com>
Cc: Ard Biesheuvel <ardb+git@google.com>, linux-efi@vger.kernel.org, 
	linux-kernel@vger.kernel.org, x86@kernel.org, 
	Ashish Kalra <Ashish.Kalra@amd.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"

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.