Re: [PATCH 2/4] KVM: nSVM: Synthesize #GP instead of SHUTDOWN if L2 attempts to clear EFER.SVME

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: userspace-reachable KVM_BUG_ON() in svm_set_efer() via KVM_SYNC_X86_SREGS
--

commit fd2bbc971d026eddf8121873792d458775b0e3cb
Author: Sean Christopherson <seanjc@google.com>

KVM: nSVM: Synthesize #GP instead of SHUTDOWN if L2 attempts to clear EFER.SVME

This commit synthesizes a #GP in set_efer() when clearing EFER_SVME while
in guest mode instead of forcing a triple fault shutdown in svm_set_efer().
It also converts the check in svm_set_efer() for clearing EFER_SVME while
is_guest_mode(vcpu) and vcpu->wants_to_run are true into a KVM_BUG_ON()
returning -EIO.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index 2df42276fd826..c3cb1575fe66e 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -626,6 +626,18 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
>  		return 1;
>  
> +	/*
> +	 * Architecturally (per the APM), clearing EFER.SVME while a guest is
> +	 * running yields undefined behavior, i.e. KVM can do literally
> +	 * anything.  Synthesize a #GP as that is the safest option for KVM,
> +	 * as mixing the current vCPU state (from L2) with L1 and the new EFER
> +	 * (if KVM simply forces the vCPU out of L2) may yield an unexpected
> +	 * end result.
> +	 */
> +	if (is_guest_mode(vcpu) &&
> +	    (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
> +		return 1;
> +
>  	efer &= ~EFER_LMA;
>  	efer |= vcpu->arch.efer & EFER_LMA;
>  
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 487897ef11dd6..a6ff9635677a1 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -227,17 +227,12 @@ int svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	if ((old_efer & EFER_SVME) != (efer & EFER_SVME)) {
>  		if (!(efer & EFER_SVME)) {
>  			/*
> -			 * Architecturally, clearing EFER.SVME while a guest is
> -			 * running yields undefined behavior, i.e. KVM can do
> -			 * literally anything.  Force the vCPU back into L1 as
> -			 * that is the safest option for KVM, but synthesize a
> -			 * triple fault (for L1!) so that KVM at least doesn't
> -			 * run random L2 code in the context of L1.  Do so if
> -			 * and only if the vCPU is actively running, e.g. to
> -			 * avoid positives if userspace is stuffing state.
> +			 * KVM is supposed to synthesize #GP if L2 attempts to
> +			 * clear SVME.
>  			 */
> -			if (is_guest_mode(vcpu) && vcpu->wants_to_run)
> -				kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> +			if (KVM_BUG_ON(is_guest_mode(vcpu) &&
> +				       vcpu->wants_to_run, vcpu->kvm))
> +				return -EIO;
>  
>  			svm_leave_nested(vcpu);
>  			/* #GP intercept is still needed for vmware backdoor */

[Severity: High]
Can userspace still reach this KVM_BUG_ON() during KVM_RUN via
KVM_SYNC_X86_SREGS when a vCPU is in guest mode?

In virt/kvm/kvm_main.c:kvm_vcpu_ioctl(), vcpu->wants_to_run is set before
calling kvm_arch_vcpu_ioctl_run():

	vcpu->wants_to_run = !READ_ONCE(vcpu->run->immediate_exit__unsafe);
	r = kvm_arch_vcpu_ioctl_run(vcpu);
	vcpu->wants_to_run = false;

Then arch/x86/kvm/x86.c:kvm_arch_vcpu_ioctl_run() syncs dirty registers
from userspace while vcpu->wants_to_run is already true:

	if (kvm_run->kvm_dirty_regs) {
		r = sync_regs(vcpu);
		if (r != 0)
			goto out;
	}

When KVM_SYNC_X86_SREGS is set in kvm_dirty_regs, the call chain
sync_regs() -> kvm_run_sync_regs_from_user() -> __set_sregs() ->
__set_sregs_common() in arch/x86/kvm/regs.c bypasses set_efer() in
arch/x86/kvm/msrs.c and calls svm_set_efer() directly:

	*mmu_reset_needed |= vcpu->arch.efer != sregs->efer;
	kvm_x86_call(set_efer)(vcpu, sregs->efer);

If userspace clears EFER_SVME in vcpu->run->s.regs.sregs.efer while
is_guest_mode(vcpu) is true, wouldn't svm_set_efer() trigger the
KVM_BUG_ON() and return -EIO (which __set_sregs_common() ignores) after
already updating vcpu->arch.efer = efer and without calling
svm_leave_nested(vcpu)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022223052.646891-1-seanjc@google.com?part=2