Received-SPF: pass (google.com: domain of linux-kernel+bounces-185445-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Date: Tue, 21 May 2024 16:00:28 -0500
From: Michael Roth <michael.roth@amd.com>
To: Sean Christopherson <seanjc@google.com>
CC: Michael Roth <mdroth@utexas.edu>, <pbonzini@redhat.com>,
	<kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<ashish.kalra@amd.com>, <thomas.lendacky@amd.com>,
	<rick.p.edgecombe@intel.com>
Subject: Re: [PATCH] KVM: SEV: Fix guest memory leak when handling guest
 requests
Message-ID: <y5yzweqbmnrbkbcmchqbszayyy4jahsc6qsfccafwr26shguyl@bldfr7cposoq>
References: <58492a1a-63bb-47d2-afef-164557d15261@redhat.com>
 <20240518150457.1033295-1-michael.roth@amd.com>
 <ZktbBRLXeOp9X6aH@google.com>
 <iqzde53xfkcpqpjvrpyetklutgqpepy3pzykwpdwyjdo7rth3m@taolptudb62c>
 <ZkvddEe3lnAlYQbQ@google.com>
 <20240521020049.tm3pa2jdi2pg4agh@amd.com>
 <ZkyrAETobNEjI4Tr@google.com>
 <qgzgdh7fqynpvu6gh6kox5rnixswtu2ewl3hiutohpndmbdo6x@kfwegt625uqh>
 <ZkzSvPGass4z4u9p@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZkzSvPGass4z4u9p@google.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 May 2024 21:00:56.8284
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: f066b04f-1d4d-4f12-33bc-08dc79d91c53
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource:
	MWH0EPF000A6734.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB8795

On Tue, May 21, 2024 at 09:58:36AM -0700, Sean Christopherson wrote:
> On Tue, May 21, 2024, Michael Roth wrote:
> > On Tue, May 21, 2024 at 07:09:04AM -0700, Sean Christopherson wrote:
> > > On Mon, May 20, 2024, Michael Roth wrote:
> > > > On Mon, May 20, 2024 at 04:32:04PM -0700, Sean Christopherson wrote:
> > > > > On Mon, May 20, 2024, Michael Roth wrote:
> > > > > > But there is a possibility that the guest will attempt access the response
> > > > > > PFN before/during that reporting and spin on an #NPF instead though. So
> > > > > > maybe the safer more repeatable approach is to handle the error directly
> > > > > > from KVM and propagate it to userspace.
> > > > > 
> > > > > I was thinking more along the lines of KVM marking the VM as dead/bugged.  
> > > > 
> > > > In practice userspace will get an unhandled exit and kill the vcpu/guest,
> > > > but we could additionally flag the guest as dead.
> > > 
> > > Honest question, does it make sense from KVM to make the VM unusable?  E.g. is
> > > it feasible for userspace to keep running the VM?  Does the page that's in a bad
> > > state present any danger to the host?
> > 
> > If the reclaim fails (which it shouldn't), then KVM has a unique situation
> > where a non-gmem guest page is in a state. In theory, if the guest/userspace
> > could somehow induce a reclaim failure, then can they potentially trick the
> > host into trying to access that same page as a shared page and induce a
> > host RMP #PF.
> > 
> > So it does seem like a good idea to force the guest to stop executing. Then
> > once the guest is fully destroyed the bad page will stay leaked so it
> > won't affect subsequent activities.
> > 
> > > 
> > > > Is there a existing mechanism for this?
> > > 
> > > kvm_vm_dead()
> > 
> > Nice, that would do the trick. I'll modify the logic to also call that
> > after a reclaim failure.
> 
> Hmm, assuming there's no scenario where snp_page_reclaim() is expected fail, and
> such a failure is always unrecoverable, e.g. has similar potential for inducing
> host RMP #PFs, then KVM_BUG_ON() is more appropriate.
> 
> Ah, and there are already WARNs in the lower level helpers.  Those WARNs should
> be KVM_BUG_ON(), because AFAICT there's no scenario where letting the VM live on
> is safe/sensible.  And unless I'm missing something, snp_page_reclaim() should
> do the private=>shared conversion, because the only reason to reclaim a page is
> to move it back to shared state.

Yes, and the code always follows up snp_page_reclaim() with
rmp_make_shared(), so it makes sense to combine the 2.

> 
> Lastly, I vote to rename host_rmp_make_shared() to kvm_rmp_make_shared() to make
> it more obvious that it's a KVM helper, whereas rmp_make_shared() is a generic
> kernel helper, i.e. _can't_ bug the VM because it doesn't (and shouldn't) have a
> pointer to the VM.

Makes sense.

> 
> E.g. end up with something like this:
> 
> /*
>  * Transition a page to hypervisor-owned/shared state in the RMP table. This
>  * should not fail under normal conditions, but leak the page should that
>  * happen since it will no longer be usable by the host due to RMP protections.
>  */
> static int kvm_rmp_make_shared(struct kvm *kvm, u64 pfn, enum pg_level level)
> {
> 	if (KVM_BUG_ON(rmp_make_shared(pfn, level), kvm)) {
> 		snp_leak_pages(pfn, page_level_size(level) >> PAGE_SHIFT);
> 		return -EIO;
> 	}
> 
> 	return 0;
> }
> 
> /*
>  * Certain page-states, such as Pre-Guest and Firmware pages (as documented
>  * in Chapter 5 of the SEV-SNP Firmware ABI under "Page States") cannot be
>  * directly transitioned back to normal/hypervisor-owned state via RMPUPDATE
>  * unless they are reclaimed first.
>  *
>  * Until they are reclaimed and subsequently transitioned via RMPUPDATE, they
>  * might not be usable by the host due to being set as immutable or still
>  * being associated with a guest ASID.
>  *
>  * Bug the VM and leak the page if reclaim fails, or if the RMP entry can't be
>  * converted back to shared, as the page is no longer usable due to RMP
>  * protections, and it's infeasible for the guest to continue on.
>  */
> static int snp_page_reclaim(struct kvm *kvm, u64 pfn)
> {
> 	struct sev_data_snp_page_reclaim data = {0};
> 	int err;
> 
> 	data.paddr = __sme_set(pfn << PAGE_SHIFT);
> 	
> 	if (KVM_BUG_ON(sev_do_cmd(SEV_CMD_SNP_PAGE_RECLAIM, &data, &err), kvm)) {

I would probably opt to use KVM_BUG() and print the PFN and firmware
error code to help with diagnosing the failure, but I think the overall
approach seems reasonable and is a safer/cleaner way to handle this
situation.

-Mike

> 		snp_leak_pages(pfn, 1);
> 		return -EIO;
> 	}
> 
> 	if (kvm_rmp_make_shared(kvm, pfn, PG_LEVEL_4K))
> 		return -EIO;
> 
> 	return 0;
> }