Received-SPF: pass (google.com: domain of linux-kernel+bounces-213965-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <16131a10-b473-41cc-a96e-d71a4d930353@amd.com>
Date: Thu, 13 Jun 2024 15:05:09 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] x86/efi: Free EFI memory map only when installing a
 new one.
Content-Language: en-US
To: Ard Biesheuvel <ardb+git@google.com>, linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
 Ard Biesheuvel <ardb@kernel.org>, Dave Young <dyoung@redhat.com>,
 Mike Rapoport <rppt@kernel.org>, Borislav Petkov <bp@alien8.de>,
 Dan Williams <dan.j.williams@intel.com>
References: <20240612135638.298882-2-ardb+git@google.com>
From: "Kalra, Ashish" <ashish.kalra@amd.com>
In-Reply-To: <20240612135638.298882-2-ardb+git@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?WGlvWCtZTDhoWURmeFBCN0lGdkV2cVRoZi8vcFNZV2U0SThJanB6WjdhSGxv?=
 =?utf-8?B?dWRxOWp4WDhwVjV4Z21KV0ZxaUUwWlBUdjhjUW1TbEViNW5qS1NlbWtrcVFW?=
 =?utf-8?B?NU83UVBHQllKQThwWEZCTXZta094N2dVUm5xYmxvNUVCdWJhaExQemRwaDBQ?=
 =?utf-8?B?SnlzdVBzYXV0c05OQTB3Z2FPWk5JR3J4WGVEVFB0cDZ2RmNGQUh3NVJQanZ1?=
 =?utf-8?B?OXAyN3RIZXdKZzFKZGVZSGRjOUQyK1VWTCtJaWJkNEErR2RZcDcxaVhnOTcv?=
 =?utf-8?B?c0xEUDlpUTdybTBndGI2ZXI0RUpwajhoRU9yRFdzdEV4T3BhelpCaG1LbXpV?=
 =?utf-8?B?ZUNCNVJnaXlWTkQwZ2V3YU9pSDZUSUtRL1NXSERBcFJIUFlYN3ZERVVFUzRu?=
 =?utf-8?B?VThIR05sMGdwNTdTUHNwYkpKcDNqNnZYQWVLOVBNeXdYVUZZNEVicFJOY1Zj?=
 =?utf-8?B?NVg5QitlNGRXUHFWZVJWRXJsOHlLRHp4bklhR3hkRTZEK1dZTmtjTUhaSEM4?=
 =?utf-8?B?Y0xnUDV6a2hNMmlaV0w1Y1IxMkFObjR3a2kzcWU2K1pLV0hxNU1XWHowL201?=
 =?utf-8?B?Y2VjWExGd2J3VFJNZG9Jbldqdi8wOTAweDNCUFhvQnE2eWlPczZ2aHI4dmRO?=
 =?utf-8?B?bzRHR3lNbkNSbVZ0TUVaL2JwZGhyKzg2NDRscFhMUitFU014WElSZUNDWUVy?=
 =?utf-8?B?UE82aVUrV2hVSmpCd3pMNkJFWDEwcTB1NEpMazVEVlJpVlUrOWNJR3NES2FI?=
 =?utf-8?B?MjFRN1FlRTgzQlVvWWk2aEIxMmUzVW1GSDdvdkVlUEE4Z0lFajFCbDlQVWZi?=
 =?utf-8?B?UlYvbVBLUTlPQzJQWjQ1YlpOV3lLV1NmRXUwNUZ6dEFEaC9kQVZXTlY1TkRt?=
 =?utf-8?B?TFFaWVVkS1ZKd2gwVVZPc3hzb2EraDNPUlBlZko0YkVvVEtHR2h2eFlkM3dT?=
 =?utf-8?B?U29HelB6OWpFcXFlOGpncEhUQ2d6cUZRckJIVjlEU0ZyWEo3aHNreUt0SUFB?=
 =?utf-8?B?QVlkSWZ0alJyQ1hOeXJpNE9YMEpISFlwZGw2Q0FtSE5rQzZNZDBBZU9ZdXk1?=
 =?utf-8?B?Wlp1Sk9yVGJUY2hkT1ZDellyMjVSenF4T2RUZEZlOS9VNWxodFpHdG1mdEFi?=
 =?utf-8?B?NUJpVCt1Y0x2L1RSZysrWU53NjRrMFVQM1ZrTTBETmgrQU9iV2lGaTB3cTFt?=
 =?utf-8?B?eDQ2eFc5V01Ub3RuVFMySkNsV2MzKzdxNzM1Q0ZHbU9kTEdRU0g5clNWOGF4?=
 =?utf-8?B?aUhMNjF1NHIvNTJuTnU0Z1V0TFlUOVYxL0hySU9FRTlmZEtqcTQ3bFB0bEt0?=
 =?utf-8?B?OHN2TzY1cTNQOUtXNjVjNjdjMjFzNGYxVzFZSzNTeE56S1ZHRG1XQTJLYjJy?=
 =?utf-8?B?UFdZUllzUTZ5K0liMTUrL1FDbVdjcXVPYUlrNUdvMDQ3OUtNMDJmSmh1bkVu?=
 =?utf-8?B?VTY3N1RHeUZObFdxVjFLMDBxaXAySUxPMUltMWpCYVAwNnc0QjM5NVNYNU5o?=
 =?utf-8?B?TVFoekE5Q05GaWRRL2RicmhZeWVHUU5RTUhRUUsxNjYrU2Q5bFc4dHBpbVhv?=
 =?utf-8?B?ckRQNWNaa0VwcWZOWjVMRHpudHViQTcvZmxON09lS2RFbUZtdlBxYmlnM3Jw?=
 =?utf-8?B?VmV3ZGpnT2xRbDd3WmVCL0J3QzJYTWNXZ2N0bUlSVkEvMGQ0QXp3ZUJScThN?=
 =?utf-8?B?RFMwSFNWaWlsZ25SUTBYY0ZCU2tROEw5V3NYMG5TcjV1VU1WWEt2VmxCK1Fa?=
 =?utf-8?B?cWtScno5Uk8xbzZXVlU5cWErNUJmbFhnMmYzL1N5cHFsNGQyanhVZXNFMytz?=
 =?utf-8?B?ajVDWFpWMTJ4dWtCVjRLSzB4WEprRW1qK1FQY1YzUmYrYnJJWUtDQ0wyWU9M?=
 =?utf-8?B?Y3BQczArWDVhanBrRDZwMVhCUU9NS3dkNXAyMlQ1bDMvNUJ2RFVVU3VoOUVx?=
 =?utf-8?B?cmFXZWt1UWlkUk4ydFJSQ2hLazF4WUZPTXdDaEo1RThUNnJRSVdEMjFMcXN0?=
 =?utf-8?B?R0RiOGorY1FPOUlPcnAwUEIxUUJ4U2RZUXdtWEpySXAzT2ZzR3N1Q2VwQmpv?=
 =?utf-8?B?Tk9ybmdiTWhNZDMweml3S2Y2TVhJeE5qeDRvcWF3VTRWZm01R1N6RWdzenhj?=
 =?utf-8?Q?KnaVNVYEKdRkvK7hSOKMq6EPe?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a971990a-a5a4-40bf-9eed-08dc8be421ce
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB2759.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jun 2024 20:05:11.7180
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: goMpuTxqeWpwITYJuJSesQVNYvQhmYl1aLYtogMjCV00wu/auj8AuYnYC27Q2QmU9AVB9Q5Uwh8ylbBhenRJbQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN6PR12MB8543

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 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(-)
>
> 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 <Ashish.Kalra@amd.com>