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

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

[Kirill A. Shutemov]

Make memremap(MEMREMAP_WB) produce encrypted/private mapping by default
unless MEMREMAP_DEC is specified.

It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.

v2:
 - Fix sparse warning;

Kirill A. Shutemov (2):
  memremap: Pass down MEMREMAP_* flags to arch_memremap_wb()
  x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default

 arch/arm/include/asm/io.h   | 2 +-
 arch/arm/mm/ioremap.c       | 2 +-
 arch/arm/mm/nommu.c         | 2 +-
 arch/riscv/include/asm/io.h | 2 +-
 arch/x86/include/asm/io.h   | 3 +++
 arch/x86/mm/ioremap.c       | 8 ++++++++
 kernel/iomem.c              | 5 +++--
 7 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.45.2

Kirill A. Shutemov (2):
  memremap: Pass down MEMREMAP_* flags to arch_memremap_wb()
  x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default

 arch/arm/include/asm/io.h   | 2 +-
 arch/arm/mm/ioremap.c       | 2 +-
 arch/arm/mm/nommu.c         | 2 +-
 arch/riscv/include/asm/io.h | 2 +-
 arch/x86/include/asm/io.h   | 3 +++
 arch/x86/mm/ioremap.c       | 8 ++++++++
 kernel/iomem.c              | 5 +++--
 7 files changed, 18 insertions(+), 6 deletions(-)

-- 
2.45.2

[PATCHv2 1/2] memremap: Pass down MEMREMAP_* flags to arch_memremap_wb()

[Kirill A. Shutemov]

x86 version of arch_memremap_wb() needs the flags to decide if the mapping
has be encrypted or decrypted.

Pass down the flag to arch_memremap_wb(). All current implementations
ignore the argument.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 arch/arm/include/asm/io.h   | 2 +-
 arch/arm/mm/ioremap.c       | 2 +-
 arch/arm/mm/nommu.c         | 2 +-
 arch/riscv/include/asm/io.h | 2 +-
 kernel/iomem.c              | 5 +++--
 5 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 1815748f5d2a..bae5edf348ef 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -381,7 +381,7 @@ void __iomem *ioremap_wc(resource_size_t res_cookie, size_t size);
 void iounmap(volatile void __iomem *io_addr);
 #define iounmap iounmap
 
-void *arch_memremap_wb(phys_addr_t phys_addr, size_t size);
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags);
 #define arch_memremap_wb arch_memremap_wb
 
 /*
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 794cfea9f9d4..9f7883e6db46 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -411,7 +411,7 @@ void __arm_iomem_set_ro(void __iomem *ptr, size_t size)
 	set_memory_ro((unsigned long)ptr, PAGE_ALIGN(size) / PAGE_SIZE);
 }
 
-void *arch_memremap_wb(phys_addr_t phys_addr, size_t size)
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags)
 {
 	return (__force void *)arch_ioremap_caller(phys_addr, size,
 						   MT_MEMORY_RW,
diff --git a/arch/arm/mm/nommu.c b/arch/arm/mm/nommu.c
index c415f3859b20..279641f0780e 100644
--- a/arch/arm/mm/nommu.c
+++ b/arch/arm/mm/nommu.c
@@ -251,7 +251,7 @@ void __iomem *pci_remap_cfgspace(resource_size_t res_cookie, size_t size)
 EXPORT_SYMBOL_GPL(pci_remap_cfgspace);
 #endif
 
-void *arch_memremap_wb(phys_addr_t phys_addr, size_t size)
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags)
 {
 	return (void *)phys_addr;
 }
diff --git a/arch/riscv/include/asm/io.h b/arch/riscv/include/asm/io.h
index 1c5c641075d2..0257f4aa7ff4 100644
--- a/arch/riscv/include/asm/io.h
+++ b/arch/riscv/include/asm/io.h
@@ -136,7 +136,7 @@ __io_writes_outs(outs, u64, q, __io_pbr(), __io_paw())
 #include <asm-generic/io.h>
 
 #ifdef CONFIG_MMU
-#define arch_memremap_wb(addr, size)	\
+#define arch_memremap_wb(addr, size, flags)	\
 	((__force void *)ioremap_prot((addr), (size), _PAGE_KERNEL))
 #endif
 
diff --git a/kernel/iomem.c b/kernel/iomem.c
index dc2120776e1c..75e61c1c6bc0 100644
--- a/kernel/iomem.c
+++ b/kernel/iomem.c
@@ -6,7 +6,8 @@
 #include <linux/ioremap.h>
 
 #ifndef arch_memremap_wb
-static void *arch_memremap_wb(resource_size_t offset, unsigned long size)
+static void *arch_memremap_wb(resource_size_t offset, unsigned long size,
+			      unsigned long flags)
 {
 #ifdef ioremap_cache
 	return (__force void *)ioremap_cache(offset, size);
@@ -91,7 +92,7 @@ void *memremap(resource_size_t offset, size_t size, unsigned long flags)
 		if (is_ram == REGION_INTERSECTS)
 			addr = try_ram_remap(offset, size, flags);
 		if (!addr)
-			addr = arch_memremap_wb(offset, size);
+			addr = arch_memremap_wb(offset, size, flags);
 	}
 
 	/*
-- 
2.45.2

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

[Kirill A. Shutemov]

Currently memremap(MEMREMAP_WB) produces decrypted/shared mapping:

memremap(MEMREMAP_WB)
  arch_memremap_wb()
    ioremap_cache()
      __ioremap_caller(.encrytped = false)

It is a bad default. On TDX guests, access via shared mapping can be
destructive[1].

Kernel already provides a way to request decrypted mapping explicitly
via MEMREMAP_DEC flag.

Make memremap(MEMREMAP_WB) produce encrypted/private mapping by default
unless MEMREMAP_DEC is specified.

It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.

[1] https://lore.kernel.org/all/20240822095122.736522-1-kirill.shutemov@linux.intel.com

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: "Maciej W. Rozycki" <macro@orcam.me.uk>
---
 arch/x86/include/asm/io.h | 3 +++
 arch/x86/mm/ioremap.c     | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/x86/include/asm/io.h b/arch/x86/include/asm/io.h
index 1d60427379c9..1a3a34b40598 100644
--- a/arch/x86/include/asm/io.h
+++ b/arch/x86/include/asm/io.h
@@ -180,6 +180,9 @@ extern void __iomem *ioremap_prot(resource_size_t offset, unsigned long size, un
 extern void __iomem *ioremap_encrypted(resource_size_t phys_addr, unsigned long size);
 #define ioremap_encrypted ioremap_encrypted
 
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags);
+#define arch_memremap_wb arch_memremap_wb
+
 /**
  * ioremap     -   map bus memory into CPU space
  * @offset:    bus address of the memory
diff --git a/arch/x86/mm/ioremap.c b/arch/x86/mm/ioremap.c
index 70b02fc61d93..f04df30f0a27 100644
--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -503,6 +503,14 @@ void iounmap(volatile void __iomem *addr)
 }
 EXPORT_SYMBOL(iounmap);
 
+void *arch_memremap_wb(phys_addr_t phys_addr, size_t size, unsigned long flags)
+{
+	if (flags & MEMREMAP_DEC)
+		return (void __force *)ioremap_cache(phys_addr, size);
+
+	return (void __force *)ioremap_encrypted(phys_addr, size);
+}
+
 /*
  * Convert a physical pointer to a virtual kernel pointer for /dev/mem
  * access
-- 
2.45.2

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

[Kirill A. Shutemov]

On Mon, Oct 21, 2024 at 01:57:21PM +0300, Kirill A. Shutemov wrote:
> Make memremap(MEMREMAP_WB) produce encrypted/private mapping by default
> unless MEMREMAP_DEC is specified.
> 
> It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.

Ping? Any feedback is welcome.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Kirill A. Shutemov]

On Mon, Oct 21, 2024 at 01:57:21PM +0300, Kirill A. Shutemov wrote:
> Make memremap(MEMREMAP_WB) produce encrypted/private mapping by default
> unless MEMREMAP_DEC is specified.
> 
> It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.

Ping? Any feedback is welcome.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Kirill A. Shutemov]

On Tue, Oct 29, 2024 at 05:11:26PM +0200, Kirill A. Shutemov wrote:
> On Mon, Oct 21, 2024 at 01:57:21PM +0300, Kirill A. Shutemov wrote:
> > Make memremap(MEMREMAP_WB) produce encrypted/private mapping by default
> > unless MEMREMAP_DEC is specified.
> > 
> > It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.
> 
> Ping? Any feedback is welcome.

The patchset fixes crash. Please consider applying.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Kirill A. Shutemov]

On Tue, Oct 29, 2024 at 05:11:26PM +0200, Kirill A. Shutemov wrote:
> On Mon, Oct 21, 2024 at 01:57:21PM +0300, Kirill A. Shutemov wrote:
> > Make memremap(MEMREMAP_WB) produce encrypted/private mapping by default
> > unless MEMREMAP_DEC is specified.
> > 
> > It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.
> 
> Ping? Any feedback is welcome.

The patchset fixes crash. Please consider applying.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Borislav Petkov]

On Mon, Oct 21, 2024 at 01:57:23PM +0300, Kirill A. Shutemov wrote:
> It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.

Do TDX guests even need EISA?

"The EISA bus saw limited use between 1988 and 1995 when it was made obsolete
by the PCI bus."

Sounds like no to me.

We usually simply stop coco guests from touching such legacy ranges:

f30470c190c2 ("x86/boot: Skip video memory access in the decompressor for SEV-ES/SNP")

so can you do that too?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

On Mon, Oct 21, 2024 at 01:57:23PM +0300, Kirill A. Shutemov wrote:
> It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.

Do TDX guests even need EISA?

"The EISA bus saw limited use between 1988 and 1995 when it was made obsolete
by the PCI bus."

Sounds like no to me.

We usually simply stop coco guests from touching such legacy ranges:

f30470c190c2 ("x86/boot: Skip video memory access in the decompressor for SEV-ES/SNP")

so can you do that too?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Kirill A. Shutemov]

On Mon, Nov 18, 2024 at 05:46:16PM +0100, Borislav Petkov wrote:
> On Mon, Oct 21, 2024 at 01:57:23PM +0300, Kirill A. Shutemov wrote:
> > It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.
> 
> Do TDX guests even need EISA?
> 
> "The EISA bus saw limited use between 1988 and 1995 when it was made obsolete
> by the PCI bus."
> 
> Sounds like no to me.
> 
> We usually simply stop coco guests from touching such legacy ranges:
> 
> f30470c190c2 ("x86/boot: Skip video memory access in the decompressor for SEV-ES/SNP")
> 
> so can you do that too?

Sure, we can workaround every place that touches such ranges. Or we can
address problem at the root and make creating decrypted/shared mappings
explicit.

Such mappings have both functional (as we see here) and security
implications (VMM can manipulate the guest memory range). We should not
create decrypted mappings by default on legacy interfaces.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Kirill A. Shutemov]

On Mon, Nov 18, 2024 at 05:46:16PM +0100, Borislav Petkov wrote:
> On Mon, Oct 21, 2024 at 01:57:23PM +0300, Kirill A. Shutemov wrote:
> > It fixes crash on kexec in TDX guests if CONFIG_EISA is enabled.
> 
> Do TDX guests even need EISA?
> 
> "The EISA bus saw limited use between 1988 and 1995 when it was made obsolete
> by the PCI bus."
> 
> Sounds like no to me.
> 
> We usually simply stop coco guests from touching such legacy ranges:
> 
> f30470c190c2 ("x86/boot: Skip video memory access in the decompressor for SEV-ES/SNP")
> 
> so can you do that too?

Sure, we can workaround every place that touches such ranges. Or we can
address problem at the root and make creating decrypted/shared mappings
explicit.

Such mappings have both functional (as we see here) and security
implications (VMM can manipulate the guest memory range). We should not
create decrypted mappings by default on legacy interfaces.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Borislav Petkov]

On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
> Sure, we can workaround every place that touches such ranges.

Every place? Which every place? I thought this is only an EISA issue?

Then clearly your changelogs need to expand considerably more what we're
*really* addressing here.

> Or we can address problem at the root and make creating decrypted/shared
> mappings explicit.

What is the problem? That KVM implicitly converts memory to shared? Why does
KVM do that an can it be fixed not to?

Doesn't sound like the guest's problem.

Or maybe this needs a lot more explanation what we're fixing here.

> Such mappings have both functional (as we see here) and security
> implications (VMM can manipulate the guest memory range). We should not
> create decrypted mappings by default on legacy interfaces.

So we're getting closer.

The changes themselves are fine but your text is missing a lot about what
we're fixing here. When I asked, I barely scratched the surface. So can we
elaborate here pls?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
> Sure, we can workaround every place that touches such ranges.

Every place? Which every place? I thought this is only an EISA issue?

Then clearly your changelogs need to expand considerably more what we're
*really* addressing here.

> Or we can address problem at the root and make creating decrypted/shared
> mappings explicit.

What is the problem? That KVM implicitly converts memory to shared? Why does
KVM do that an can it be fixed not to?

Doesn't sound like the guest's problem.

Or maybe this needs a lot more explanation what we're fixing here.

> Such mappings have both functional (as we see here) and security
> implications (VMM can manipulate the guest memory range). We should not
> create decrypted mappings by default on legacy interfaces.

So we're getting closer.

The changes themselves are fine but your text is missing a lot about what
we're fixing here. When I asked, I barely scratched the surface. So can we
elaborate here pls?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Kirill A. Shutemov]

On Thu, Nov 21, 2024 at 12:49:52PM +0100, Borislav Petkov wrote:
> On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
> > Sure, we can workaround every place that touches such ranges.
> 
> Every place? Which every place? I thought this is only an EISA issue?

I looked at other places where we call memremap(MEMREMAP_WB) such as
acpi_wakeup_cpu(). We actually get encrypted/private mapping for this
callsite despite __ioremap_caller() being called encrypted == false.
This happens because of IORES_MAP_ENCRYPTED check in __ioremap_caller().

So we depend on the BIOS here. The EISA problem happens because the
target memory is in !IORES_MAP_ENCRYPTED memory.

It's hard to say if any other memremap(MEMREMAP_WB) would trigger the
issue. And what will happen after next BIOS update.

> Then clearly your changelogs need to expand considerably more what we're
> *really* addressing here.
> 
> > Or we can address problem at the root and make creating decrypted/shared
> > mappings explicit.
> 
> What is the problem? That KVM implicitly converts memory to shared? Why does
> KVM do that an can it be fixed not to?
> 
> Doesn't sound like the guest's problem.

Well, the problem is on the both sides.

VMM behaviour on such accesses is not specified in any spec. AFAIK all
current VMM implementations do this implicit conversion.

I think it has to be fixed. VMMs (not only KVM) should not silently
convert memory to shared. But VMMs cannot make memory access to go away.
The only option they have is to inject #VE instead indicating bogus
access. At this point it becomes a guest problem.

It will get fixed in VMMs naturally when TDX Connect gets enabled.
With a secure device assigned to a TD, VMM would loose the ability to
convert memory on its own. The guest would have to unlock the memory
first. This will make implicit conversion impossible.

But it also means guest should never initiate shared access without
explicit conversion. Otherwise #VE will crash it.

> Or maybe this needs a lot more explanation what we're fixing here.
> 
> > Such mappings have both functional (as we see here) and security
> > implications (VMM can manipulate the guest memory range). We should not
> > create decrypted mappings by default on legacy interfaces.
> 
> So we're getting closer.
> 
> The changes themselves are fine but your text is missing a lot about what
> we're fixing here. When I asked, I barely scratched the surface. So can we
> elaborate here pls?

What about this:

x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default

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.

This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
enabled.
 
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Kirill A. Shutemov]

On Thu, Nov 21, 2024 at 12:49:52PM +0100, Borislav Petkov wrote:
> On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
> > Sure, we can workaround every place that touches such ranges.
> 
> Every place? Which every place? I thought this is only an EISA issue?

I looked at other places where we call memremap(MEMREMAP_WB) such as
acpi_wakeup_cpu(). We actually get encrypted/private mapping for this
callsite despite __ioremap_caller() being called encrypted == false.
This happens because of IORES_MAP_ENCRYPTED check in __ioremap_caller().

So we depend on the BIOS here. The EISA problem happens because the
target memory is in !IORES_MAP_ENCRYPTED memory.

It's hard to say if any other memremap(MEMREMAP_WB) would trigger the
issue. And what will happen after next BIOS update.

> Then clearly your changelogs need to expand considerably more what we're
> *really* addressing here.
> 
> > Or we can address problem at the root and make creating decrypted/shared
> > mappings explicit.
> 
> What is the problem? That KVM implicitly converts memory to shared? Why does
> KVM do that an can it be fixed not to?
> 
> Doesn't sound like the guest's problem.

Well, the problem is on the both sides.

VMM behaviour on such accesses is not specified in any spec. AFAIK all
current VMM implementations do this implicit conversion.

I think it has to be fixed. VMMs (not only KVM) should not silently
convert memory to shared. But VMMs cannot make memory access to go away.
The only option they have is to inject #VE instead indicating bogus
access. At this point it becomes a guest problem.

It will get fixed in VMMs naturally when TDX Connect gets enabled.
With a secure device assigned to a TD, VMM would loose the ability to
convert memory on its own. The guest would have to unlock the memory
first. This will make implicit conversion impossible.

But it also means guest should never initiate shared access without
explicit conversion. Otherwise #VE will crash it.

> Or maybe this needs a lot more explanation what we're fixing here.
> 
> > Such mappings have both functional (as we see here) and security
> > implications (VMM can manipulate the guest memory range). We should not
> > create decrypted mappings by default on legacy interfaces.
> 
> So we're getting closer.
> 
> The changes themselves are fine but your text is missing a lot about what
> we're fixing here. When I asked, I barely scratched the surface. So can we
> elaborate here pls?

What about this:

x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default

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.

This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
enabled.
 
-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Tom Lendacky]

On 12/10/24 07:26, Kirill A. Shutemov wrote:
> On Thu, Nov 21, 2024 at 12:49:52PM +0100, Borislav Petkov wrote:
>> On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
>>> Sure, we can workaround every place that touches such ranges.
>>
>> Every place? Which every place? I thought this is only an EISA issue?
> 
> I looked at other places where we call memremap(MEMREMAP_WB) such as
> acpi_wakeup_cpu(). We actually get encrypted/private mapping for this
> callsite despite __ioremap_caller() being called encrypted == false.
> This happens because of IORES_MAP_ENCRYPTED check in __ioremap_caller().
> 
> So we depend on the BIOS here. The EISA problem happens because the
> target memory is in !IORES_MAP_ENCRYPTED memory.
> 
> It's hard to say if any other memremap(MEMREMAP_WB) would trigger the
> issue. And what will happen after next BIOS update.
> 
>> Then clearly your changelogs need to expand considerably more what we're
>> *really* addressing here.
>>
>>> Or we can address problem at the root and make creating decrypted/shared
>>> mappings explicit.
>>
>> What is the problem? That KVM implicitly converts memory to shared? Why does
>> KVM do that an can it be fixed not to?
>>
>> Doesn't sound like the guest's problem.
> 
> Well, the problem is on the both sides.
> 
> VMM behaviour on such accesses is not specified in any spec. AFAIK all
> current VMM implementations do this implicit conversion.
> 
> I think it has to be fixed. VMMs (not only KVM) should not silently
> convert memory to shared. But VMMs cannot make memory access to go away.
> The only option they have is to inject #VE instead indicating bogus
> access. At this point it becomes a guest problem.
> 
> It will get fixed in VMMs naturally when TDX Connect gets enabled.
> With a secure device assigned to a TD, VMM would loose the ability to
> convert memory on its own. The guest would have to unlock the memory
> first. This will make implicit conversion impossible.
> 
> But it also means guest should never initiate shared access without
> explicit conversion. Otherwise #VE will crash it.
> 
>> Or maybe this needs a lot more explanation what we're fixing here.
>>
>>> Such mappings have both functional (as we see here) and security
>>> implications (VMM can manipulate the guest memory range). We should not
>>> create decrypted mappings by default on legacy interfaces.
>>
>> So we're getting closer.
>>
>> The changes themselves are fine but your text is missing a lot about what
>> we're fixing here. When I asked, I barely scratched the surface. So can we
>> elaborate here pls?
> 
> What about this:
> 
> x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default
> 
> Currently memremap(MEMREMAP_WB) can produce decrypted/shared mapping:
> 
> memremap(MEMREMAP_WB)
>   arch_memremap_wb()
>     ioremap_cache()
>       __ioremap_caller(.encrytped = false)

That's because try_ram_remap() invokes arch_memremap_can_ram_remap()
which is returning false (for some reason).

When arch_memremap_can_ram_remap() returns false, ioremap_cache() is
invoked. ioremap() should provide shared mappings unless specifically
requested to provide an encrypted mapping (via encrypted parameter) or
if __ioremap_check_mem() determines that an encrypted mapping is needed.

Can logic be added to arch_memremap_can_ram_remap() to return true for
the cases that TDX is having issues with?

Thanks,
Tom

> 
> 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.
> 
> This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
> enabled.
>  

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

[Tom Lendacky]

On 12/10/24 07:26, Kirill A. Shutemov wrote:
> On Thu, Nov 21, 2024 at 12:49:52PM +0100, Borislav Petkov wrote:
>> On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
>>> Sure, we can workaround every place that touches such ranges.
>>
>> Every place? Which every place? I thought this is only an EISA issue?
> 
> I looked at other places where we call memremap(MEMREMAP_WB) such as
> acpi_wakeup_cpu(). We actually get encrypted/private mapping for this
> callsite despite __ioremap_caller() being called encrypted == false.
> This happens because of IORES_MAP_ENCRYPTED check in __ioremap_caller().
> 
> So we depend on the BIOS here. The EISA problem happens because the
> target memory is in !IORES_MAP_ENCRYPTED memory.
> 
> It's hard to say if any other memremap(MEMREMAP_WB) would trigger the
> issue. And what will happen after next BIOS update.
> 
>> Then clearly your changelogs need to expand considerably more what we're
>> *really* addressing here.
>>
>>> Or we can address problem at the root and make creating decrypted/shared
>>> mappings explicit.
>>
>> What is the problem? That KVM implicitly converts memory to shared? Why does
>> KVM do that an can it be fixed not to?
>>
>> Doesn't sound like the guest's problem.
> 
> Well, the problem is on the both sides.
> 
> VMM behaviour on such accesses is not specified in any spec. AFAIK all
> current VMM implementations do this implicit conversion.
> 
> I think it has to be fixed. VMMs (not only KVM) should not silently
> convert memory to shared. But VMMs cannot make memory access to go away.
> The only option they have is to inject #VE instead indicating bogus
> access. At this point it becomes a guest problem.
> 
> It will get fixed in VMMs naturally when TDX Connect gets enabled.
> With a secure device assigned to a TD, VMM would loose the ability to
> convert memory on its own. The guest would have to unlock the memory
> first. This will make implicit conversion impossible.
> 
> But it also means guest should never initiate shared access without
> explicit conversion. Otherwise #VE will crash it.
> 
>> Or maybe this needs a lot more explanation what we're fixing here.
>>
>>> Such mappings have both functional (as we see here) and security
>>> implications (VMM can manipulate the guest memory range). We should not
>>> create decrypted mappings by default on legacy interfaces.
>>
>> So we're getting closer.
>>
>> The changes themselves are fine but your text is missing a lot about what
>> we're fixing here. When I asked, I barely scratched the surface. So can we
>> elaborate here pls?
> 
> What about this:
> 
> x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default
> 
> Currently memremap(MEMREMAP_WB) can produce decrypted/shared mapping:
> 
> memremap(MEMREMAP_WB)
>   arch_memremap_wb()
>     ioremap_cache()
>       __ioremap_caller(.encrytped = false)

That's because try_ram_remap() invokes arch_memremap_can_ram_remap()
which is returning false (for some reason).

When arch_memremap_can_ram_remap() returns false, ioremap_cache() is
invoked. ioremap() should provide shared mappings unless specifically
requested to provide an encrypted mapping (via encrypted parameter) or
if __ioremap_check_mem() determines that an encrypted mapping is needed.

Can logic be added to arch_memremap_can_ram_remap() to return true for
the cases that TDX is having issues with?

Thanks,
Tom

> 
> 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.
> 
> This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
> enabled.
>  

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Kirill A. Shutemov]

On Thu, Dec 12, 2024 at 08:23:07AM -0600, Tom Lendacky wrote:
> On 12/10/24 07:26, Kirill A. Shutemov wrote:
> > On Thu, Nov 21, 2024 at 12:49:52PM +0100, Borislav Petkov wrote:
> >> On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
> >>> Sure, we can workaround every place that touches such ranges.
> >>
> >> Every place? Which every place? I thought this is only an EISA issue?
> > 
> > I looked at other places where we call memremap(MEMREMAP_WB) such as
> > acpi_wakeup_cpu(). We actually get encrypted/private mapping for this
> > callsite despite __ioremap_caller() being called encrypted == false.
> > This happens because of IORES_MAP_ENCRYPTED check in __ioremap_caller().
> > 
> > So we depend on the BIOS here. The EISA problem happens because the
> > target memory is in !IORES_MAP_ENCRYPTED memory.
> > 
> > It's hard to say if any other memremap(MEMREMAP_WB) would trigger the
> > issue. And what will happen after next BIOS update.
> > 
> >> Then clearly your changelogs need to expand considerably more what we're
> >> *really* addressing here.
> >>
> >>> Or we can address problem at the root and make creating decrypted/shared
> >>> mappings explicit.
> >>
> >> What is the problem? That KVM implicitly converts memory to shared? Why does
> >> KVM do that an can it be fixed not to?
> >>
> >> Doesn't sound like the guest's problem.
> > 
> > Well, the problem is on the both sides.
> > 
> > VMM behaviour on such accesses is not specified in any spec. AFAIK all
> > current VMM implementations do this implicit conversion.
> > 
> > I think it has to be fixed. VMMs (not only KVM) should not silently
> > convert memory to shared. But VMMs cannot make memory access to go away.
> > The only option they have is to inject #VE instead indicating bogus
> > access. At this point it becomes a guest problem.
> > 
> > It will get fixed in VMMs naturally when TDX Connect gets enabled.
> > With a secure device assigned to a TD, VMM would loose the ability to
> > convert memory on its own. The guest would have to unlock the memory
> > first. This will make implicit conversion impossible.
> > 
> > But it also means guest should never initiate shared access without
> > explicit conversion. Otherwise #VE will crash it.
> > 
> >> Or maybe this needs a lot more explanation what we're fixing here.
> >>
> >>> Such mappings have both functional (as we see here) and security
> >>> implications (VMM can manipulate the guest memory range). We should not
> >>> create decrypted mappings by default on legacy interfaces.
> >>
> >> So we're getting closer.
> >>
> >> The changes themselves are fine but your text is missing a lot about what
> >> we're fixing here. When I asked, I barely scratched the surface. So can we
> >> elaborate here pls?
> > 
> > What about this:
> > 
> > x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default
> > 
> > Currently memremap(MEMREMAP_WB) can produce decrypted/shared mapping:
> > 
> > memremap(MEMREMAP_WB)
> >   arch_memremap_wb()
> >     ioremap_cache()
> >       __ioremap_caller(.encrytped = false)
> 
> That's because try_ram_remap() invokes arch_memremap_can_ram_remap()
> which is returning false (for some reason).

In EISA case try_ram_remap() is not called. is_ram is REGION_DISJOINT

> When arch_memremap_can_ram_remap() returns false, ioremap_cache() is
> invoked. ioremap() should provide shared mappings unless specifically
> requested to provide an encrypted mapping (via encrypted parameter) or
> if __ioremap_check_mem() determines that an encrypted mapping is needed.

I don't propose changing ioremap() behaviour. It's about memremap().

> Can logic be added to arch_memremap_can_ram_remap() to return true for
> the cases that TDX is having issues with?

It will not help EISA case, because we don't get this path.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Kirill A. Shutemov]

On Thu, Dec 12, 2024 at 08:23:07AM -0600, Tom Lendacky wrote:
> On 12/10/24 07:26, Kirill A. Shutemov wrote:
> > On Thu, Nov 21, 2024 at 12:49:52PM +0100, Borislav Petkov wrote:
> >> On Tue, Nov 19, 2024 at 10:21:05AM +0200, Kirill A. Shutemov wrote:
> >>> Sure, we can workaround every place that touches such ranges.
> >>
> >> Every place? Which every place? I thought this is only an EISA issue?
> > 
> > I looked at other places where we call memremap(MEMREMAP_WB) such as
> > acpi_wakeup_cpu(). We actually get encrypted/private mapping for this
> > callsite despite __ioremap_caller() being called encrypted == false.
> > This happens because of IORES_MAP_ENCRYPTED check in __ioremap_caller().
> > 
> > So we depend on the BIOS here. The EISA problem happens because the
> > target memory is in !IORES_MAP_ENCRYPTED memory.
> > 
> > It's hard to say if any other memremap(MEMREMAP_WB) would trigger the
> > issue. And what will happen after next BIOS update.
> > 
> >> Then clearly your changelogs need to expand considerably more what we're
> >> *really* addressing here.
> >>
> >>> Or we can address problem at the root and make creating decrypted/shared
> >>> mappings explicit.
> >>
> >> What is the problem? That KVM implicitly converts memory to shared? Why does
> >> KVM do that an can it be fixed not to?
> >>
> >> Doesn't sound like the guest's problem.
> > 
> > Well, the problem is on the both sides.
> > 
> > VMM behaviour on such accesses is not specified in any spec. AFAIK all
> > current VMM implementations do this implicit conversion.
> > 
> > I think it has to be fixed. VMMs (not only KVM) should not silently
> > convert memory to shared. But VMMs cannot make memory access to go away.
> > The only option they have is to inject #VE instead indicating bogus
> > access. At this point it becomes a guest problem.
> > 
> > It will get fixed in VMMs naturally when TDX Connect gets enabled.
> > With a secure device assigned to a TD, VMM would loose the ability to
> > convert memory on its own. The guest would have to unlock the memory
> > first. This will make implicit conversion impossible.
> > 
> > But it also means guest should never initiate shared access without
> > explicit conversion. Otherwise #VE will crash it.
> > 
> >> Or maybe this needs a lot more explanation what we're fixing here.
> >>
> >>> Such mappings have both functional (as we see here) and security
> >>> implications (VMM can manipulate the guest memory range). We should not
> >>> create decrypted mappings by default on legacy interfaces.
> >>
> >> So we're getting closer.
> >>
> >> The changes themselves are fine but your text is missing a lot about what
> >> we're fixing here. When I asked, I barely scratched the surface. So can we
> >> elaborate here pls?
> > 
> > What about this:
> > 
> > x86/mm: Make memremap(MEMREMAP_WB) map memory as encrypted by default
> > 
> > Currently memremap(MEMREMAP_WB) can produce decrypted/shared mapping:
> > 
> > memremap(MEMREMAP_WB)
> >   arch_memremap_wb()
> >     ioremap_cache()
> >       __ioremap_caller(.encrytped = false)
> 
> That's because try_ram_remap() invokes arch_memremap_can_ram_remap()
> which is returning false (for some reason).

In EISA case try_ram_remap() is not called. is_ram is REGION_DISJOINT

> When arch_memremap_can_ram_remap() returns false, ioremap_cache() is
> invoked. ioremap() should provide shared mappings unless specifically
> requested to provide an encrypted mapping (via encrypted parameter) or
> if __ioremap_check_mem() determines that an encrypted mapping is needed.

I don't propose changing ioremap() behaviour. It's about memremap().

> Can logic be added to arch_memremap_can_ram_remap() to return true for
> the cases that TDX is having issues with?

It will not help EISA case, because we don't get this path.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Kirill A. Shutemov]

Borislav,

Is there anything I need to do to get this patchset merged?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Kirill A. Shutemov]

Borislav,

Is there anything I need to do to get this patchset merged?

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Borislav Petkov]

On Mon, Jan 13, 2025 at 12:03:22PM +0200, Kirill A. Shutemov wrote:
> Borislav,
> 
> Is there anything I need to do to get this patchset merged?

Your updated commit message here:

https://lore.kernel.org/r/bz3vlh3lri7yfckdkddopwhsgvkmizhw5q6ecomgeba7q22ufp@ntu2kugiho4o

Reads better, yes.

> This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
> enabled.

s/This change fixes/Fix/

In imperative tone pls.

And that sentence begs the question: does this need to go to stable?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

On Mon, Jan 13, 2025 at 12:03:22PM +0200, Kirill A. Shutemov wrote:
> Borislav,
> 
> Is there anything I need to do to get this patchset merged?

Your updated commit message here:

https://lore.kernel.org/r/bz3vlh3lri7yfckdkddopwhsgvkmizhw5q6ecomgeba7q22ufp@ntu2kugiho4o

Reads better, yes.

> This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
> enabled.

s/This change fixes/Fix/

In imperative tone pls.

And that sentence begs the question: does this need to go to stable?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Kirill A. Shutemov]

On Mon, Jan 13, 2025 at 11:32:38AM +0100, Borislav Petkov wrote:
> On Mon, Jan 13, 2025 at 12:03:22PM +0200, Kirill A. Shutemov wrote:
> > Borislav,
> > 
> > Is there anything I need to do to get this patchset merged?
> 
> Your updated commit message here:
> 
> https://lore.kernel.org/r/bz3vlh3lri7yfckdkddopwhsgvkmizhw5q6ecomgeba7q22ufp@ntu2kugiho4o
> 
> Reads better, yes.
> 
> > This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
> > enabled.
> 
> s/This change fixes/Fix/
> 
> In imperative tone pls.

Okay.

> And that sentence begs the question: does this need to go to stable?

Right.

It is hard to pin down Fixes commit as the bug is inherited from initial
encrypted/decrypted handling implementation, but it is not useful to
backport it all the way.

I'll add

Cc: stable@vger.kernel.org # 6.11+

6.11 is where kexec handling for TDX was introduced.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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

[Kirill A. Shutemov]

On Mon, Jan 13, 2025 at 11:32:38AM +0100, Borislav Petkov wrote:
> On Mon, Jan 13, 2025 at 12:03:22PM +0200, Kirill A. Shutemov wrote:
> > Borislav,
> > 
> > Is there anything I need to do to get this patchset merged?
> 
> Your updated commit message here:
> 
> https://lore.kernel.org/r/bz3vlh3lri7yfckdkddopwhsgvkmizhw5q6ecomgeba7q22ufp@ntu2kugiho4o
> 
> Reads better, yes.
> 
> > This change fixes the crash on kexec in TDX guests if CONFIG_EISA is
> > enabled.
> 
> s/This change fixes/Fix/
> 
> In imperative tone pls.

Okay.

> And that sentence begs the question: does this need to go to stable?

Right.

It is hard to pin down Fixes commit as the bug is inherited from initial
encrypted/decrypted handling implementation, but it is not useful to
backport it all the way.

I'll add

Cc: stable@vger.kernel.org # 6.11+

6.11 is where kexec handling for TDX was introduced.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv