Message-ID: <myzpconz5z7xf4j7fnofb5zgxv7fc3evxhcys6wnqlao2c3xkj@aoapggj7lchq>
Date: Fri, 13 Dec 2024 13:59:12 +0200
From: "Kirill A. Shutemov" <kirill@...temov.name>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
 	Borislav Petkov <bp@...en8.de>, Thomas Gleixner <tglx@...utronix.de>,
 	Ingo Molnar <mingo@...hat.com>,
 Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
 	"H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>,
 	Albert Ou <aou@...s.berkeley.edu>, Alexei Starovoitov <ast@...nel.org>,
 	Andrea Parri <parri.andrea@...il.com>, Arnd Bergmann <arnd@...db.de>,
 	Daniel Borkmann <daniel@...earbox.net>,
 Eric Chan <ericchancf@...gle.com>, 	Jason Gunthorpe <jgg@...pe.ca>,
 Kai Huang <kai.huang@...el.com>,
 	Kefeng Wang <wangkefeng.wang@...wei.com>,
 Kent Overstreet <kent.overstreet@...ux.dev>,
 	Palmer Dabbelt <palmer@...osinc.com>,
 Paul Walmsley <paul.walmsley@...ive.com>,
 	Russell King <linux@...linux.org.uk>,
 Samuel Holland <samuel.holland@...ive.com>,
 	Suren Baghdasaryan <surenb@...gle.com>, Yuntao Wang <ytcoode@...il.com>,
 	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-riscv@...ts.infradead.org, 	Ashish Kalra <ashish.kalra@....com>,
 "Maciej W. Rozycki" <macro@...am.me.uk>
Subject: Re: [PATCHv2 2/2] x86/mm: Make memremap(MEMREMAP_WB) map memory as
 encrypted by default

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

Powered by blists - more mailing lists