linux-kernel - Re: [PATCH v12 29/29] KVM: SEV: Provide support for SNP_EXTENDED_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: <5f0e74f2-6261-d836-5161-a525ed9c497a@amd.com>
Date: Thu, 11 Apr 2024 08:33:47 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Michael Roth <michael.roth@....com>, kvm@...r.kernel.org
Cc: linux-coco@...ts.linux.dev, linux-mm@...ck.org,
 linux-crypto@...r.kernel.org, x86@...nel.org, linux-kernel@...r.kernel.org,
 tglx@...utronix.de, mingo@...hat.com, jroedel@...e.de, hpa@...or.com,
 ardb@...nel.org, pbonzini@...hat.com, seanjc@...gle.com,
 vkuznets@...hat.com, jmattson@...gle.com, luto@...nel.org,
 dave.hansen@...ux.intel.com, slp@...hat.com, pgonda@...gle.com,
 peterz@...radead.org, srinivas.pandruvada@...ux.intel.com,
 rientjes@...gle.com, dovmurik@...ux.ibm.com, tobin@....com, bp@...en8.de,
 vbabka@...e.cz, kirill@...temov.name, ak@...ux.intel.com,
 tony.luck@...el.com, sathyanarayanan.kuppuswamy@...ux.intel.com,
 alpergun@...gle.com, jarkko@...nel.org, ashish.kalra@....com,
 nikunj.dadhania@....com, pankaj.gupta@....com, liam.merwick@...cle.com
Subject: Re: [PATCH v12 29/29] KVM: SEV: Provide support for
 SNP_EXTENDED_GUEST_REQUEST NAE event

On 3/29/24 17:58, Michael Roth wrote:
> Version 2 of GHCB specification added support for the SNP Extended Guest
> Request Message NAE event. This event serves a nearly identical purpose
> to the previously-added SNP_GUEST_REQUEST event, but allows for
> additional certificate data to be supplied via an additional
> guest-supplied buffer to be used mainly for verifying the signature of
> an attestation report as returned by firmware.
> 
> This certificate data is supplied by userspace, so unlike with
> SNP_GUEST_REQUEST events, SNP_EXTENDED_GUEST_REQUEST events are first
> forwarded to userspace via a KVM_EXIT_VMGEXIT exit type, and then the
> firmware request is made only afterward.
> 
> Implement handling for these events.
> 
> Since there is a potential for race conditions where the
> userspace-supplied certificate data may be out-of-sync relative to the
> reported TCB or VLEK that firmware will use when signing attestation
> reports, make use of the synchronization mechanisms wired up to the
> SNP_{PAUSE,RESUME}_ATTESTATION SEV device ioctls such that the guest
> will be told to retry the request while attestation has been paused due
> to an update being underway on the system.
> 
> Signed-off-by: Michael Roth <michael.roth@....com>
> ---
>   Documentation/virt/kvm/api.rst | 26 ++++++++++++
>   arch/x86/include/asm/sev.h     |  4 ++
>   arch/x86/kvm/svm/sev.c         | 75 ++++++++++++++++++++++++++++++++++
>   arch/x86/kvm/svm/svm.h         |  3 ++
>   arch/x86/virt/svm/sev.c        | 21 ++++++++++
>   include/uapi/linux/kvm.h       |  6 +++
>   6 files changed, 135 insertions(+)
> 

> +static int snp_complete_ext_guest_req(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct vmcb_control_area *control;
> +	struct kvm *kvm = vcpu->kvm;
> +	sev_ret_code fw_err = 0;
> +	int vmm_ret;
> +
> +	vmm_ret = vcpu->run->vmgexit.ext_guest_req.ret;
> +	if (vmm_ret) {
> +		if (vmm_ret == SNP_GUEST_VMM_ERR_INVALID_LEN)
> +			vcpu->arch.regs[VCPU_REGS_RBX] =
> +				vcpu->run->vmgexit.ext_guest_req.data_npages;
> +		goto abort_request;
> +	}
> +
> +	control = &svm->vmcb->control;
> +
> +	if (!__snp_handle_guest_req(kvm, control->exit_info_1, control->exit_info_2,
> +				    &fw_err))
> +		vmm_ret = SNP_GUEST_VMM_ERR_GENERIC;
> +
> +	/*
> +	 * Give errors related to stale transactions precedence to provide more
> +	 * potential options for servicing firmware while guests are running.
> +	 */
> +	if (snp_transaction_is_stale(svm->snp_transaction_id))
> +		vmm_ret = SNP_GUEST_VMM_ERR_BUSY;

I think having this after the call to the SEV firmware will cause an 
issue. If the firmware has performed the attestation request 
successfully in the __snp_handle_guest_req() call, then it will have 
incremented the sequence number. If you return busy, then the sev-guest 
driver will attempt to re-issue the request with the original sequence 
number which will now fail. That failure will then be propagated back to 
the sev-guest driver which will then disable the VMPCK key.

So I think you need to put this before the call to firmware.

Thanks,
Tom

> +