Re: [PATCHv3 2/2] x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default

[Tom Lendacky]

On 1/14/25 01:27, Kirill A. Shutemov wrote:
> On Mon, Jan 13, 2025 at 02:47:56PM -0600, Tom Lendacky wrote:
>> On 1/13/25 07:14, Kirill A. Shutemov wrote:
>>> Currently memremap(MEMREMAP_WB) can produce decrypted/shared mapping:
>>>
>>> memremap(MEMREMAP_WB)
>>>   arch_memremap_wb()
>>>     ioremap_cache()
>>>       __ioremap_caller(.encrytped = false)
>>>
>>> In such cases, the IORES_MAP_ENCRYPTED flag on the memory will determine
>>> if the resulting mapping is encrypted or decrypted.
>>>
>>> Creating a decrypted mapping without explicit request from the caller is
>>> risky:
>>>
>>>   - It can inadvertently expose the guest's data and compromise the
>>>     guest.
>>>
>>>   - Accessing private memory via shared/decrypted mapping on TDX will
>>>     either trigger implicit conversion to shared or #VE (depending on
>>>     VMM implementation).
>>>
>>>     Implicit conversion is destructive: subsequent access to the same
>>>     memory via private mapping will trigger a hard-to-debug #VE crash.
>>>
>>> The kernel already provides a way to request decrypted mapping
>>> explicitly via the MEMREMAP_DEC flag.
>>>
>>> Modify memremap(MEMREMAP_WB) to produce encrypted/private mapping by
>>> default unless MEMREMAP_DEC is specified.
>>>
>>> Fix the crash due to #VE on kexec in TDX guests if CONFIG_EISA is enabled.
>>
>> This patch causes my bare-metal system to crash during boot when using
>> mem_encrypt=on:
>>
>> [    2.392934] efi: memattr: Entry type should be RuntimeServiceCode/Data
>> [    2.393731] efi: memattr: ! 0x214c42f01f1162a-0xee70ac7bd1a9c629 [type=2028324321|attr=0x6590648fa4209879]
> 
> Could you try if this helps?
> 
> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c
> index c38b1a335590..b5051dcb7c1d 100644
> --- a/drivers/firmware/efi/memattr.c
> +++ b/drivers/firmware/efi/memattr.c
> @@ -160,7 +160,7 @@ int __init efi_memattr_apply_permissions(struct mm_struct *mm,
>  	if (WARN_ON(!efi_enabled(EFI_MEMMAP)))
>  		return 0;
>  
> -	tbl = memremap(efi_mem_attr_table, tbl_size, MEMREMAP_WB);
> +	tbl = memremap(efi_mem_attr_table, tbl_size, MEMREMAP_WB | MEMREMAP_DEC);

Well that would work for SME where EFI tables/data are not encrypted,
but will break for SEV where EFI tables/data are encrypted.

Thanks,
Tom

>  	if (!tbl) {
>  		pr_err("Failed to map EFI Memory Attributes table @ 0x%lx\n",
>  		       efi_mem_attr_table);