Re: [PATCH v2 2/6] KVM: selftests: Add a test to verify SEV {en,de}crypt debug ioctls

[Sean Christopherson <seanjc@google.com>]

+lists

On Tue, Jun 23, 2026, Michael Roth wrote:
> On Thu, Jun 18, 2026 at 10:18:03AM -0500, Michael Roth wrote:
> > On Wed, Jun 17, 2026 at 06:59:24PM -0700, Sean Christopherson wrote:
> > > off-list in case this is double-ungood.
> > > 
> > > I ran this on an SNP host today (I think this is the first time I've run the full
> > > test on an SNP host), and it explodes with RMP violation #PFs in weird ways.  I
> > > don't _think_ it's a KVM bug?  Because I get the RMP violations even without ever
> > > running an SNP guest.  I'm using kvm/next, at commit ef057cbf825e ("KVM: x86/mmu:
> > > Ensure hugepage is in by slot before checking max mapping level")
> > > 
> > > It might be related to edge cases around pages, or maybe large sizes, as the test
> > > passes if I comment out the testcases that focus on the larger sizes.  I haven't
> > > narrowed it down further than this (reboots aren't exactly fast).
> > > 
> > > What's especially odd is that on most of the crashes, there's no apparent reason
> > > for the RMP violation, as the RMP says the page is unassigned.  I did get one
> > > crash (#2 below) where the RMP entry was non-zero.  That one was after booting
> > > an SNP VM, but it was quite some time after shutting down that VM.  The RMP dump
> > > is equally confusing to me, because AFAICT the RMP entry is corrupted.
> > > 
> > > Mike/Tom, can you try and repro and debug?  I don't have the bandwidth to dig
> > > deep on this, and even if I did, I suspect it's beyond my abilities to debug.
> > 
> > Hi Sean,
> > 
> > I was able to reproduce this after running the test in a loop for a few
> > minutes after a clean boot (trace below).
> > 
> > Not making much sense to be ATM either, but might be related to RMP table
> > memory itself. Will keep investigating.
> 
> It looks like this was a recent regression caused by changes to how the
> temp buffers are allocated. I tested the below revert patch ~14 hours
> and the issue seems to be fixed (I generally was reproducing it within
> an couple hours or so max so it seems good).

Thanks for root causing!
 
> Planning to submit upstream today if there's no objections/concerns.

I object :-)

> From: Michael Roth <michael.roth@amd.com>
> Date: Mon, 22 Jun 2026 22:47:03 -0500
> Subject: [PATCH] Revert "KVM: SEV: Allocate only as many bytes as needed for
>  temp crypt buffers"
> 
> When SNP is enabled, the data passed to firmware must be contained
> within pages that have been transferred to firmware ownership via the
> corresponding RMP table updates. 

IIUC, and I'm pretty sure I do at this point, this is wrong, or at least *very*
misleading.  Nothing in KVM performs RMP table updates, and so saying "the data
passed to firmware must be contained within pages that have been transferred to
firmware ownership" is completely inaccurate.

Piecing together the changelog and diff, and my own observations, my understanding
is that **firmware** modifies the RMP to *temporarily* take ownership of the page
while performing the {DE,EN}CRYPT operation.  So there's no requirement that the
pages be in any specific state, the only "requirement" is that software needs to
either prevent concurrent accesses or be prepared to handle spurious RMP #PFs due
to the temporarily RMP modifications.

SNP_DBG_DECRYPT does require software to convert the destination to be a firmware
page: "The firmware also checks that the destination page is a Firmware page."
But this is about SEV_DBG_{DE,EN}CRYPT, not SNP_DBG_ENCRYPT.

> This is not compatible with uses kmalloc() allocations since kernel accesses

As is the statement about kmalloc().  kmalloc() of a PAGE_SIZE is a-ok, because
the kernel will always hand out a full page.

> to other allocations within that page will trigger an RMP fault and crash the

And it's specificaly about other *accesses*, not simply other allocations.

> host. Fix this by moving back to page-based allocations.o

Always forcing full page allocations is overkill for SEV and SEV-ES, and robs us
of the opportunity to document that SNP+ is special, which is especially important
because AFAICT, none of the specs are so kind as to document this "minor" behavior.

Rather than fully revert, just force PAGE_SIZE allocations if the host supports SNP.
I haven't run anywhere near 14 hours, but I was also getting failures on every
single run of the test, and I ran the test 100 times without problem.

However, this only addresses the case where KVM is using temporary buffers.  For
small, nicely aligned operations, a misbehaving userspace could induce a crash by
coercing the kernel into accessing the to-be-{de,en}crypted page via a kernel
mapping while the crypo op is in-progess.  I don't see a less awful option than
forcing KVM to use the "slow" path, e.g. if the problem is limited to the dest
for a decrypt operation:

diff --git arch/x86/kvm/svm/sev.c arch/x86/kvm/svm/sev.c
index 87025d0d2f91..7e3334c90c57 100644
--- arch/x86/kvm/svm/sev.c
+++ arch/x86/kvm/svm/sev.c
@@ -1406,7 +1406,8 @@ static int sev_dbg_crypt(struct kvm *kvm, struct kvm_sev_cmd *argp,
                sev_clflush_pages(&src_p, 1);
                sev_clflush_pages(&dst_p, 1);
 
-               if (IS_ALIGNED(src, 16) && IS_ALIGNED(dst, 16) && IS_ALIGNED(len, 16))
+               if (IS_ALIGNED(src, 16) && IS_ALIGNED(dst, 16) && IS_ALIGNED(len, 16) &&
+                   (!cc_platform_has(CC_ATTR_HOST_SEV_SNP) || cmd != KVM_SEV_DBG_DECRYPT))
                        ret = sev_issue_dbg_cmd(kvm,
                                                __sme_page_pa(src_p) + s_off,
                                                __sme_page_pa(dst_p) + d_off,

But if firmware temporarily converts *both* pages, then we're "stuck" because at
some point KVM needs to actually target the correct guest-owned page.  The only
option I can think of is to require capable(CAP_SYS_BOOT), *if* the above doesn't
suffice.

So what exactly is the behavior here?  Because I can't find anything in the specs,
I can only make educated guesses based on the SNP_DBG_DECRYPT documentation.

---
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 23 Jun 2026 12:21:56 -0700
Subject: [PATCH] KVM: SEV: Allocate full pages for {DE,EN}CRYPT ops on
 SNP-enabled hosts

When {de,en}crypting memory of an SEV or SEV-ES guest on an SNP-enabled
host via a temporary buffer, allocate a full 4KiB page for the buffer to
ensure the kernel won't concurrently access the page that's being
{de,en}crypted.  On SNP-enabled platforms, firmware modifies the RMP to
temporarily take ownership of the to-be-{de,en}crypted page, and so using
a sub-allocation results in unexpected (and seemingly spurious) RMP #PF
violations due to software attempting to access a firmware-owned page.

Note, it is unclear whether the RMP updates are considered architectural,
or an implementation quirk, as none of the documentation for the non-SNP
DBG_{DE,EN}CRYPT commands say anything about RMP updates, nor does the SNP
specific firmware ABI spec.

Fixes: 4c735bf1bc22 ("KVM: SEV: Allocate only as many bytes as needed for temp crypt buffers")
Debugged-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/svm/sev.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 74fb15551e83..87025d0d2f91 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -1280,6 +1280,16 @@ static void *sev_dbg_crypt_slow_alloc(struct page *page, unsigned long __va,
 	if (WARN_ON_ONCE((*pa & PAGE_MASK) != ((*pa + *nr_bytes - 1) & PAGE_MASK)))
 		return NULL;
 
+	/*
+	 * If SNP is enabled, i.e. the RMP is active, allocate a full page to
+	 * prevent concurrent accesses to the page.  Firmware modifies the RMP
+	 * to temporarily take ownership of the page while the {DE,EN}CRYPT
+	 * operation is in-progress, and so concurrent software accesses will
+	 * encounter seemingly spurious RMP #PF violations
+	 */
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		return kmalloc(PAGE_SIZE, GFP_KERNEL);
+
 	return kmalloc(*nr_bytes, GFP_KERNEL);
 }
 

base-commit: 9d4853b044beefa21c4ee3e18c40653601a64ced
--