On 1/14/25 09:06, Tom Lendacky wrote:
On 1/14/25 08:44, Kirill A. Shutemov wrote:
On Tue, Jan 14, 2025 at 08:33:39AM -0600, Tom Lendacky wrote:
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.
Hm. Why would it break for SEV? It brings the situation back to what it was before the patch.
Ah, true. I can try it and see how much further SME gets. Hopefully it doesn't turn into a whack-a-mole thing.
Unfortunately, it is turning into a whack-a-mole thing.
But it looks the following works for SME:
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c index 3c36f3f5e688..ff3cd5fc8508 100644 --- a/arch/x86/mm/ioremap.c +++ b/arch/x86/mm/ioremap.c @@ -505,7 +505,7 @@ EXPORT_SYMBOL(iounmap);
void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags) { - if (flags & MEMREMAP_DEC) + if (flags & MEMREMAP_DEC || cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) return (void __force *)ioremap_cache(phys_addr, size);
return (void __force *)ioremap_encrypted(phys_addr, size);
I haven't had a chance to test the series on SEV, yet.
Thanks, Tom
Thanks, Tom
Note that that __ioremap_caller() would still check io_desc.flags before mapping it as decrypted.
On Tue, Jan 14, 2025 at 10:54:53AM -0600, Tom Lendacky wrote:
On 1/14/25 09:06, Tom Lendacky wrote:
On 1/14/25 08:44, Kirill A. Shutemov wrote:
On Tue, Jan 14, 2025 at 08:33:39AM -0600, Tom Lendacky wrote:
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.
Hm. Why would it break for SEV? It brings the situation back to what it was before the patch.
Ah, true. I can try it and see how much further SME gets. Hopefully it doesn't turn into a whack-a-mole thing.
Unfortunately, it is turning into a whack-a-mole thing.
But it looks the following works for SME:
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c index 3c36f3f5e688..ff3cd5fc8508 100644 --- a/arch/x86/mm/ioremap.c +++ b/arch/x86/mm/ioremap.c @@ -505,7 +505,7 @@ EXPORT_SYMBOL(iounmap); void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags) {
- if (flags & MEMREMAP_DEC)
- if (flags & MEMREMAP_DEC || cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT)) return (void __force *)ioremap_cache(phys_addr, size);
return (void __force *)ioremap_encrypted(phys_addr, size);
I haven't had a chance to test the series on SEV, yet.
Please do.
I am okay with the change above. Borislav, is it acceptable direction for you?
On Wed, Jan 15, 2025 at 12:23:54PM +0200, Kirill A. Shutemov wrote:
I am okay with the change above. Borislav, is it acceptable direction for you?
Yes, Tom and I have been talking offlist about making the *map code figure out itself what type of encryption setting it should use, based on the platform. It'll need a proper analysis, though, what all the possible usages are to see whether that scheme would be adequeate.
Something to experiment with after the merge window.
Thx.
linux-stable-mirror@lists.linaro.org
-
Borislav Petkov -
Kirill A. Shutemov -
Tom Lendacky