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

[Michael Roth]

On Wed, Jun 24, 2026 at 02:24:24PM -0700, Sean Christopherson wrote:
> On Tue, Jun 23, 2026, Sean Christopherson wrote:
> > On Tue, Jun 23, 2026, Michael Roth wrote:
> > > On Tue, Jun 23, 2026 at 07:55:27PM +0000, Sean Christopherson wrote:
> > > since your proposed fix seems nicer and more complete anyway.
> > 
> > It's missing a case though :-(
> 
> Ah, but so is the revert, and it's not a coincidence that the old code didn't
> bounce buffer the destination for KVM_SEV_DBG_ENCRYPT.

Yah I was realizing this too :( Though I'm not sure why we set
'guest_owned' for DBG_DECRYPT... I'm not even sure it makes sense to
write decrypted data into guest memory since it would only be usable
if the range was decrypted to begin with... in most cases this likely
wouldn't be guest memory.

So maybe the bounce buffer approach would work for DBG_DECRYPT at
least, but yah...I think the issue is more general...

> 
> > The final write for ENCRYPT writes to the destination directly:
> > 
> > 		r = sev_issue_dbg_cmd(kvm, __sme_set(__pa(buf)), dst_pa,
> > 				      nr_bytes, KVM_SEV_DBG_ENCRYPT, err);
> > 
> > More at the bottom.
> 
> Once more, with feeling...
> 
> > > > 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.
> > > 
> > > Not sure I understand fully, but snp_populate_cmd_buf_desc_list() in the
> > > firmware driver requires the transiton for the destination page that we
> > > write to both for 'encrypt' as well as 'decrypt'. What the issue with
> > > using a temp buffer as the destination for the 'encrypt' case?
> > 
> > The problem is if KVM does NOT use a temporary buffer, which is the "fast" path
> > where src, dst, and len are all 16-byte aligned.  In that case, KVM does inline
> > {de,en}cryption between src and dst.  But those pages are also mapped into
> > userspace (they have to be, since KVM pins them via gup()), which means that
> > userspace could feed the same pages to a different chunk of the kernel that
> > reads/write userspace memory via gup() (or gup-like equivalent).
> > 
> > E.g. pread()/read() on shmem-backed memory.
> 
> pread() doesn't work, because the source or destination needs to be a userspace
> address, and so any RMP #PFs will get eaten.  But copy_file_range() makes the
> world go kablooie.

I was hoping we could at least rely on kernel accesses going through GUP
and maybe doing something like unmapping and freezing folio refcount
to block concurrent access to the page, or read-only protections that
would force the GUP to fail and just fail the concurrent userspace-requested
operation...

copy_file_range() bypasses the GUP, but freezing the refcount or locking
the folio would cause it to spin until the folio was released. So maybe
that's an option?

This is similar to what's been proposed for gmem hugetlb support when
splitting/rebuilding hugepages as part of the conversion path, where
we need to guard against concurrent access by both kernel/userspace, so
maybe it's not too hacky?

> 
> Weirdly, I haven't been able to get a splat of any kind, the host just goes
> AWOL and reboots.  But I'm very confident RMP #PFs are the issue, because doing
> the same operation via writes in userspace yields:
> 
>     sev_dbg_test-24385   [049] d....  1892.853197: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
>     sev_dbg_test-24385   [049] d....  1892.853202: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
>     sev_dbg_test-24385   [049] d....  1892.853204: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
>     sev_dbg_test-24385   [049] d....  1892.853206: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
>     sev_dbg_test-24385   [049] d....  1892.853207: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
>     sev_dbg_test-24385   [049] d....  1892.853209: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
>     sev_dbg_test-24385   [049] d....  1892.853211: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
>     sev_dbg_test-24385   [049] d....  1892.853212: page_fault_user: address=0x7fc63f8fe000 ip=0x41cb44 error_code=0x80000007
> 
> 
> > Looking at the full picture, isn't this a bug in the PSP driver that affects all
> > commands?  With pre-SNP VMs, "guest owned" isn't really a thing, since nothing
> > guarantees that the destination page is accessible *only* by the guest.
> > 
> > If we rip that out and always bounce-buffer, then KVM won't have to force its
> > own bounce buffering of sub-page lengths for SNP, and that would eliminate the
> > race I described above.
> > 
> > Completely untested (I'll give this a whirl tomorrow).
> 
> So this doesn't work.  Dredging up some knowledge of how this all works, I'm
> fairly certain the issue is that the encryption is salted with the physical address,
> and so encrypting at X instead of Y will yield different ciphertext.  I.e. any
> command that encrypts the contents in the destination (or in theory, decrypts,
> though AFAICT none of the commands do so) *can't* be bounce buffered.

Yes, sorry for not noticing that detail sooner. It really throws a
wrench in things =\

> 
> I don't see a way to salvage SEV/SEV-ES on SNP systems, short of requiring
> CAP_SYS_BOOT or some other elevated permission to do *anything*.  Or redo SEV/SEV-ES
> support to require guest_memfd for operations that require putting pages into
> Firmware state.

Yah, short of maybe the above approach, I don't see any way around it atm :(
If you think it's worth pursuing though I can give that a shot on my
end.

Thanks,

Mike