linux-kernel - Re: [PATCH v1 1/5] KVM: SEV: Provide support for SNP_GUEST

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240621161711.fvx5kjugfdhl2r3p@amd.com>
Date: Fri, 21 Jun 2024 11:17:11 -0500
From: Michael Roth <michael.roth@....com>
To: Liam Merwick <liam.merwick@...cle.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "linux-coco@...ts.linux.dev"
	<linux-coco@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "jroedel@...e.de" <jroedel@...e.de>,
	"thomas.lendacky@....com" <thomas.lendacky@....com>, "pgonda@...gle.com"
	<pgonda@...gle.com>, "ashish.kalra@....com" <ashish.kalra@....com>,
	"bp@...en8.de" <bp@...en8.de>, "pankaj.gupta@....com" <pankaj.gupta@....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 Fri, Jun 21, 2024 at 03:52:35PM +0000, Liam Merwick wrote:
> On 21/06/2024 14:40, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@....com>
> > 
> > Version 2 of GHCB specification added support for the SNP Guest Request
> > Message NAE event. The event allows for an SEV-SNP guest to make
> > requests to the SEV-SNP firmware through hypervisor using the
> > SNP_GUEST_REQUEST API defined in the SEV-SNP firmware specification.
> > 
> > This is used by guests primarily to request attestation reports from
> > firmware. There are other request types are available as well, but the
> > specifics of what guest requests are being made are opaque to the
> > hypervisor, which only serves as a proxy for the guest requests and
> > firmware responses.
> > 
> > Implement handling for these events.
> > 
> > Signed-off-by: Brijesh Singh <brijesh.singh@....com>
> > Co-developed-by: Alexey Kardashevskiy <aik@....com>
> > Signed-off-by: Alexey Kardashevskiy <aik@....com>
> > Co-developed-by: Ashish Kalra <ashish.kalra@....com>
> > Signed-off-by: Ashish Kalra <ashish.kalra@....com>
> > Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
> > [mdr: ensure FW command failures are indicated to guest, drop extended
> >   request handling to be re-written as separate patch, massage commit]
> > Signed-off-by: Michael Roth <michael.roth@....com>
> > Message-ID: <20240501085210.2213060-19-michael.roth@....com>
> > Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> > ---
> >   arch/x86/kvm/svm/sev.c         | 69 ++++++++++++++++++++++++++++++++++
> >   include/uapi/linux/sev-guest.h |  9 +++++
> >   2 files changed, 78 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index df8818759698..7338b987cadd 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -19,6 +19,7 @@
> >   #include <linux/misc_cgroup.h>
> >   #include <linux/processor.h>
> >   #include <linux/trace_events.h>
> > +#include <uapi/linux/sev-guest.h>
> >   
> >   #include <asm/pkru.h>
> >   #include <asm/trapnr.h>
> > @@ -3321,6 +3322,10 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> >   		if (!sev_snp_guest(vcpu->kvm) || !kvm_ghcb_sw_scratch_is_valid(svm))
> >   			goto vmgexit_err;
> >   		break;
> > +	case SVM_VMGEXIT_GUEST_REQUEST:
> > +		if (!sev_snp_guest(vcpu->kvm))
> > +			goto vmgexit_err;
> > +		break;
> >   	default:
> >   		reason = GHCB_ERR_INVALID_EVENT;
> >   		goto vmgexit_err;
> > @@ -3939,6 +3944,67 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
> >   	return ret;
> >   }
> >   
> > +static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_gpa)
> > +{
> > +	struct sev_data_snp_guest_request data = {0};
> > +	struct kvm *kvm = svm->vcpu.kvm;
> > +	kvm_pfn_t req_pfn, resp_pfn;
> > +	sev_ret_code fw_err = 0;
> > +	int ret;
> > +
> > +	if (!sev_snp_guest(kvm) || !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;
> > +
> > +	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;
> > +	}
> > +
> > +	data.gctx_paddr = __psp_pa(to_kvm_sev_info(kvm)->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 (ret)
> > +		return ret;
> 
> 
> Should this be goto release_req; ?
> Does resp_pfn need to be dealt with as well?

Argh... yes. I folded some helper functions into here and missed updating
some of the return sites. I'll respond to this patch with a revised version.
Sorry for the noise.

-Mike