Received-SPF: pass (google.com: domain of linux-kernel+bounces-212816-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240612135638.298882-2-ardb+git@google.com>
In-Reply-To: <20240612135638.298882-2-ardb+git@google.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Thu, 13 Jun 2024 10:28:05 +0200
Message-ID: <CAMj1kXEgAStastV+sMhXt0pUmvM=nr9D-oDctwb8iiYxt9+fAQ@mail.gmail.com>
Subject: Re: [PATCH v3] x86/efi: Free EFI memory map only when installing a
 new one.
To: Ard Biesheuvel <ardb+git@google.com>
Cc: linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org, 
	Ashish Kalra <Ashish.Kalra@amd.com>, 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"

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.