Re: [PATCH 58/60] kvm: svm: Invoke a specified VMPL level VMSA for the vCPU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The MSR protocol handler for GHCB_MSR_VMPL_REQ propagates `-EINVAL` to KVM_RUN when a requested VMPL plane does not exist, causing the VM to crash.
- [High] Requesting a switch to the currently active VMPL puts the active vCPU plane into the STOPPED state, permanently hanging the vCPU thread.
- [High] Missing array_index_nospec() on guest-controlled VMPL index creates a Spectre v1 vulnerability.
--

commit 13abcdff43b65b23c1d479626cab3e83b513a2c8
Author: Tom Lendacky <thomas.lendacky@amd.com>

kvm: svm: Invoke a specified VMPL level VMSA for the vCPU

Implement the SNP Run VMPL NAE event and MSR protocol to allow a guest to
request a different VMPL level VMSA be run for the vCPU. This allows the
guest to "call" an SVSM to process an SVSM request.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0b531eda062f3..70a06769a4100 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
[ ... ]
> @@ -4573,6 +4577,45 @@ static void sev_get_apic_ids(struct vcpu_svm *svm)
>  	kvfree(desc);
>  }
>  
> +static int __sev_snp_run_vmpl(struct vcpu_svm *svm, unsigned int vmpl)
> +{
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +	struct kvm_vcpu *target = vcpu->common->vcpus[vmpl];
> +	struct vcpu_svm *target_svm = to_svm(target);
> +
> +	if (!target)
> +		return -EINVAL;
> +
> +	/* Mark current plane as stopped so it is not selected */
> +	kvm_set_mp_state(target, KVM_MP_STATE_RUNNABLE);
> +	/* In case KVM_REQ_UPDATE_PROTECTED_GUEST_STATE is set - mark the new VMSA as runnable */
> +	target_svm->sev_es.snp_ap_runnable = true;
> +	kvm_vcpu_set_plane_runnable(target);
> +	kvm_vcpu_set_plane_stopped(vcpu);

[Severity: High]
If a guest requests a switch to the VMPL level it is already executing on,
target and vcpu will point to the same object.  Will this sequence
cause the vCPU to hang?

Since kvm_vcpu_set_plane_runnable(target) is immediately followed by
kvm_vcpu_set_plane_stopped(vcpu), the plane's state would be overwritten
to STOPPED, indefinitely suspending the vCPU.

Should there be an explicit check for target == vcpu to handle this?

> +
> +	kvm_make_request(KVM_REQ_PLANE_RESCHED, vcpu);
> +
> +	return 1;
> +}
> +
> +static int sev_snp_run_vmpl(struct vcpu_svm *svm)
> +{
> +	struct ghcb *ghcb = svm->sev_es.ghcb;
> +	struct kvm_vcpu *vcpu = &svm->vcpu;
> +	unsigned int vmpl;
> +
> +	vmpl = lower_32_bits(svm->vmcb->control.exit_info_1);
> +	if (vmpl >= SVM_SEV_VMPL_MAX) {
> +		vcpu_unimpl(vcpu, "vmgexit: invalid VMPL level [%u] from guest\n", vmpl);
> +		return -EINVAL;
> +	}

[Severity: High]
Since vmpl is guest-controlled and used as an index into the
vcpu->common->vcpus[vmpl] array in __sev_snp_run_vmpl(), is there a risk
of speculative out-of-bounds execution here?

Should vmpl be sanitized with array_index_nospec() after this bounds
check to prevent a potential Spectre v1 vulnerability?

> +
> +	ghcb_set_sw_exit_info_1(ghcb, 0);
> +	ghcb_set_sw_exit_info_2(ghcb, 0);
> +
> +	return __sev_snp_run_vmpl(svm, vmpl);
> +}
> +
[ ... ]
> @@ -4684,6 +4727,27 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>  
>  		ret = snp_begin_psc_msr(svm, control->ghcb_gpa);
>  		break;
> +	case GHCB_MSR_VMPL_REQ: {
> +		unsigned int vmpl;
> +
> +		if (!sev_snp_guest(vcpu->kvm))
> +			goto out_terminate;
> +
> +		vmpl = get_ghcb_msr_bits(svm, GHCB_MSR_VMPL_LEVEL_MASK, GHCB_MSR_VMPL_LEVEL_POS);
> +
> +		set_ghcb_msr_bits(svm, 0, GHCB_MSR_VMPL_ERROR_MASK, GHCB_MSR_VMPL_ERROR_POS);
> +		set_ghcb_msr_bits(svm, 0, GHCB_MSR_VMPL_RSVD_MASK, GHCB_MSR_VMPL_RSVD_POS);
> +		set_ghcb_msr_bits(svm, GHCB_MSR_VMPL_RESP, GHCB_MSR_INFO_MASK, GHCB_MSR_INFO_POS);
> +
> +		if (vmpl >= SVM_SEV_VMPL_MAX) {
> +			vcpu_unimpl(vcpu, "vmgexit: invalid VMPL level [%u] from guest\n", vmpl);
> +			set_ghcb_msr_bits(svm, 1, GHCB_MSR_VMPL_ERROR_MASK, GHCB_MSR_VMPL_ERROR_POS);
> +			break;
> +		}

[Severity: High]
Similar to sev_snp_run_vmpl(), does this guest-controlled vmpl index require
array_index_nospec() after the bounds check to mitigate Spectre v1?

> +
> +		ret = __sev_snp_run_vmpl(svm, vmpl);
> +		break;

[Severity: High]
If target == NULL inside __sev_snp_run_vmpl(), it returns -EINVAL which
is then assigned to ret here and propagated up.  Will this cause the
KVM_RUN ioctl to return -EINVAL to userspace?

Since QEMU treats negative returns from KVM_RUN as fatal errors and aborts
the VM, could a less privileged guest level (e.g., VMPL3) exploit this to
intentionally crash the VM and disrupt more privileged levels?

Should the MSR handler trap the negative return, set GHCB_MSR_VMPL_ERROR_MASK
in the response, and return 1 to safely resume the guest instead?

> +	}
>  	case GHCB_MSR_TERM_REQ: {
>  		u64 reason_set, reason_code;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144252.351443-1-joro@8bytes.org?part=58