Date: Wed, 26 Jun 2024 12:42:31 -0500
From: Michael Roth <michael.roth@....com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <kvm@...r.kernel.org>, <linux-coco@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, <x86@...nel.org>, <pbonzini@...hat.com>,
	<jroedel@...e.de>, <thomas.lendacky@....com>, <pgonda@...gle.com>,
	<ashish.kalra@....com>, <bp@...en8.de>, <pankaj.gupta@....com>,
	<liam.merwick@...cle.com>, Brijesh Singh <brijesh.singh@....com>, "Alexey
 Kardashevskiy" <aik@....com>
Subject: Re: [PATCH v1 1/5] KVM: SEV: Provide support for SNP_GUEST_REQUEST
 NAE event

On Wed, Jun 26, 2024 at 10:13:44AM -0700, Sean Christopherson wrote:
> On Wed, Jun 26, 2024, Michael Roth wrote:
> > On Wed, Jun 26, 2024 at 06:58:09AM -0700, Sean Christopherson wrote:
> > > [*] https://lore.kernel.org/all/20240229025759.1187910-1-stevensd@google.com
> > > 
> > > > +	if (is_error_noslot_pfn(req_pfn))
> > > > +		return -EINVAL;
> > > > +
> > > > +	resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
> > > > +	if (is_error_noslot_pfn(resp_pfn)) {
> > > > +		ret = EINVAL;
> > > > +		goto release_req;
> > > > +	}
> > > > +
> > > > +	if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) {
> > > > +		ret = -EINVAL;
> > > > +		kvm_release_pfn_clean(resp_pfn);
> > > > +		goto release_req;
> > > > +	}
> > > 
> > > I don't see how this is safe.  KVM holds no locks, i.e. can't guarantee that the
> > > resp_pfn stays private for the duration of the operation.  And on the opposite
> > 
> > When the page is set to private with asid=0,immutable=true arguments,
> > this puts the page in a special 'firmware-owned' state that specifically
> > to avoid any changes to the page state happening from under the ASPs feet.
> > The only way to switch the page to any other state at this point is to
> > issue the SEV_CMD_SNP_PAGE_RECLAIM request to the ASP via
> > snp_page_reclaim().
> >
> > I could see the guest shooting itself in the foot by issuing 2 guest
> > requests with the same req_pfn/resp_pfn, but on the KVM side whichever
> > request issues rmp_make_private() first would succeed, and then the
> > 2nd request would generate an EINVAL to userspace.
> > 
> > In that sense, rmp_make_private()/snp_page_reclaim() sort of pair to
> > lock/unlock a page that's being handed to the ASP. But this should be
> > better documented either way.
> 
> What about the host kernel though?  I don't see anything here that ensures resp_pfn
> isn't "regular" memory, i.e. that ensure the page isn't being concurrently accessed
> by the host kernel (or some other userspace process).
> 
> Or is the "private" memory still accessible by the host?

It's accessible, but it is immutable according to RMP table, so so it would
require KVM to be elsewhere doing a write to the page, but that seems possible
if the guest is misbehaved. So I do think the RMP #PF concerns are warranted,
and that looking at using KVM-allocated intermediary/"bounce" pages to pass to
firmware is definitely worth looking into for v2 as that's just about the
safest way to guarantee nothing else will be writing to the page after
it gets set to immutable/firmware-owned.

> 
> > > resp_pfn stays private for the duration of the operation.  And on the opposite
> > > side, KVM can't guarantee that resp_pfn isn't being actively used by something
> > > in the kernel, e.g. KVM might induce an unexpected #PF(RMP).
> > > 
> > > Why can't KVM require that the response/destination page already be private?  I'm
> > 
> > Hmm. This is a bit tricky. The GHCB spec states:
> > 
> >   The Guest Request NAE event requires two unique pages, one page for the
> >   request and one page for the response. Both pages must be assigned to the
> >   hypervisor (shared). The guest must supply the guest physical address of
> >   the pages (i.e., page aligned) as input.
> > 
> >   The hypervisor must translate the guest physical address (GPA) of each
> >   page into a system physical address (SPA). The SPA is used to verify that
> >   the request and response pages are assigned to the hypervisor.
> > 
> > At least for req_pfn, this makes sense because the header of the message
> > is actually plain text, and KVM does need to parse it to read the message
> > type from the header. It's just the req/resp payload that are encrypted
> > by the guest/firmware, i.e. it's not relying on hardware-based memory
> > encryption in this case.
> > 
> > Because of that though, I think we could potential address this by
> > allocating the req/resp page as separate pages outside of guest memory,
> > and simply copy the contents to/from the GPAs provided by the guest.
> > I'll look more into this approach.
> > 
> > If we go that route I think some of the concerns above go away as well,
> > but we might still need to a lock or something to serialize access to
> > these separate/intermediate pages to avoid needed to allocate them every
> > time or per-request.
> > 
> > > also somewhat confused by the reclaim below.  If KVM converts the response page
> > > back to shared, doesn't that clobber the data?  If so, how does the guest actually
> > > get the response?  I feel like I'm missing something.
> > 
> > In this case we're just setting it immutable/firmware-owned, which just
> > happens to be equivalent (in terms of the RMP table) to a guest-owned page,
> > but with rmp_entry.ASID=0/rmp_entry.immutable=true instead of
> > rmp_entry.ASID=<guest asid>/rmp_entry.immutable=false. So the contents remain
> > intact/readable after the reclaim.
> 
> Ah, I see the @asid=0 now.  The @asid=0,@immutable=true should be a separate API,
> because IIUC, this always holds true:

That makes sense. I'll look at introducing rmp_make_firmware() as part of
the next revision.

-Mike

> 
> 	!asid == immutable
> 
> E.g.
> 
> static int rmp_assign_pfn(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immutable)
> {
> 	struct rmp_state state;
> 
> 	memset(&state, 0, sizeof(state));
> 	state.assigned = 1;
> 	state.asid = asid;
> 	state.immutable = immutable;
> 	state.gpa = gpa;
> 	state.pagesize = PG_LEVEL_TO_RMP(level);
> 
> 	return rmpupdate(pfn, &state);	
> }
> 
> /* Transition a page to guest-owned/private state in the RMP table. */
> int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid)
> {
> 	if (WARN_ON_ONCE(!asid))
> 		return -EIO;
> 
> 	return rmp_assign_pfn(pfn, gpa, level, asid, false);
> }
> EXPORT_SYMBOL_GPL(rmp_make_private);
> 
> /* Transition a page to AMD-SP owned state in the RMP table. */
> int rmp_make_firmware(u64 pfn, u64 gpa)
> {
> 	return rmp_assign_pfn(pfn, gpa, PG_LEVEL_4K, 0, true);
> }
> EXPORT_SYMBOL_GPL(rmp_make_firmware);

Powered by blists - more mailing lists