Received-SPF: pass (google.com: domain of linux-kernel+bounces-184278-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: Mon, 20 May 2024 17:50:44 -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: <iqzde53xfkcpqpjvrpyetklutgqpepy3pzykwpdwyjdo7rth3m@taolptudb62c>
References: <58492a1a-63bb-47d2-afef-164557d15261@redhat.com>
 <20240518150457.1033295-1-michael.roth@amd.com>
 <ZktbBRLXeOp9X6aH@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZktbBRLXeOp9X6aH@google.com>
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 May 2024 22:51:16.5645
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 548c9d37-5f8a-4214-b8b6-08dc791f5b86
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:
	SN1PEPF00036F41.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB7884

On Mon, May 20, 2024 at 07:17:13AM -0700, Sean Christopherson wrote:
> This needs a
> 
>   From: Michael Roth <michael.roth@amd.com>
> 
> otherwise Author will be assigned to your @utexas.edu email.

Thanks, I hadn't considered that. My work email issue seems to be
resolved now, but will keep that in mind if I ever need to use a
fallback again.

> 
> On Sat, May 18, 2024, Michael Roth wrote:
> > Before forwarding guest requests to firmware, KVM takes a reference on
> > the 2 pages the guest uses for its request/response buffers. Make sure
> > to release these when cleaning up after the request is completed.
> > 
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> 
> ...
> 
> > @@ -3970,14 +3980,11 @@ static int __snp_handle_guest_req(struct kvm *kvm, gpa_t req_gpa, gpa_t resp_gpa
> >  		return ret;
> >  
> >  	ret = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, fw_err);
> > -	if (ret)
> > -		return ret;
> >  
> > -	ret = snp_cleanup_guest_buf(&data);
> > -	if (ret)
> > -		return ret;
> > +	if (snp_cleanup_guest_buf(&data))
> > +		return -EINVAL;
> 
> EINVAL feels wrong.  The input was completely valid.  Also, forwarding the error

Yah, EIO seems more suitable here.

> to the guest doesn't seem like the right thing to do if KVM can't reclaim the
> response PFN.  Shouldn't that be fatal to the VM?

The thinking here is that pretty much all guest request failures will be
fatal to the guest being able to continue. At least, that's definitely
true for attestation. So reporting the error to the guest would allow that
failure to be propagated along by handling in the guest where it would
presumably be reported a little more clearly to the guest owner, at
which point the guest would most likely terminate itself anyway.

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.

But the GHCB spec does require that the firmware response code for
SNP_GUEST_REQUEST be passed directly to the guest via lower 32-bits of
SW_EXITINFO2, so we'd still want handling to pass that error on to the
guest, so I made some changes to retain that behavior.

> 
> > -	return 0;
> > +	return ret;
> 
> I find the setup/cleanup split makes this code harder to read, not easier.  It
> won't be pretty no matter waht due to the potential RMP failures, but IMO this
> is easier to follow:

It *might* make more sense to split things out into helpers when extended
guest requests are implemented, but for the patch in question I agree
what you have below is clearer. I also went a step further and moved
__snp_handle_guest_req() back into snp_handle_guest_req() as well to
simplify the logic for always passing firmware errors back to the guest.

I'll post a v2 of the fixup with these changes added. But I've also
pushed it here for reference:

  https://github.com/mdroth/linux/commit/8ceab17950dc5f1b94231037748104f7c31752f8
  (from https://github.com/mdroth/linux/commits/kvm-next-snp-fixes2/)

and here's the original PATCH 17/19 with all pending fixes squashed in:

  https://github.com/mdroth/linux/commit/b4f51e38da22a2b163c546cb2a3aefd04446b3c7
  (from https://github.com/mdroth/linux/commits/kvm-next-snp-fixes2-squashed/)
  (also retested attestation with simulated failures and double-checked
   for clang warnings with W=1)
  
Thanks!

-Mike

> 
> 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> 	struct sev_data_snp_guest_request data = {0};
> 	kvm_pfn_t req_pfn, resp_pfn;
> 	int ret;
> 
> 	if (!sev_snp_guest(kvm))
> 		return -EINVAL;
> 
> 	if (!PAGE_ALIGNED(req_gpa) || !PAGE_ALIGNED(resp_gpa))
> 		return -EINVAL;
> 
> 	req_pfn = gfn_to_pfn(kvm, gpa_to_gfn(req_gpa));
> 	if (is_error_noslot_pfn(req_pfn))
> 		return -EINVAL;
> 
> 	ret = -EINVAL;
> 
> 	resp_pfn = gfn_to_pfn(kvm, gpa_to_gfn(resp_gpa));
> 	if (is_error_noslot_pfn(resp_pfn))
> 		goto release_req;
> 
> 	if (rmp_make_private(resp_pfn, 0, PG_LEVEL_4K, 0, true)) {
> 		kvm_release_pfn_clean(resp_pfn);
> 		goto release_req;
> 	}
> 
> 	data.gctx_paddr = __psp_pa(sev->snp_context);
> 	data.req_paddr = __sme_set(req_pfn << PAGE_SHIFT);
> 	data.res_paddr = __sme_set(resp_pfn << PAGE_SHIFT);
> 	ret = sev_issue_cmd(kvm, SEV_CMD_SNP_GUEST_REQUEST, &data, fw_err);
> 
> 	if (snp_page_reclaim(resp_pfn) ||
> 	    rmp_make_shared(resp_pfn, PG_LEVEL_4K))
> 		ret = ret ?: -EIO;
> 	else
> 		kvm_release_pfn_dirty(resp_pfn);
> release_req:
> 	kvm_release_pfn_clean(req_pfn);
> 	return ret;
> 
>