linux-kernel - Re: [PATCH v4 13/26] KVM: nSVM: Unify handling of VMRUN failures with proper cleanup

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: <aYU6P2qNEpRVWllL@google.com>
Date: Thu, 5 Feb 2026 16:47:59 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH v4 13/26] KVM: nSVM: Unify handling of VMRUN failures with
 proper cleanup

On Thu, Jan 15, 2026, Yosry Ahmed wrote:
> @@ -983,6 +991,8 @@ static void __nested_svm_vmexit(struct vcpu_svm *svm)
>  	struct vmcb *vmcb01 = svm->vmcb01.ptr;
>  	struct kvm_vcpu *vcpu = &svm->vcpu;
>  
> +	WARN_ON_ONCE(is_guest_mode(vcpu));
> +
>  	svm->nested.vmcb12_gpa = 0;
>  	svm->nested.ctl.nested_cr3 = 0;
>  
> @@ -1006,6 +1016,19 @@ static void __nested_svm_vmexit(struct vcpu_svm *svm)
>  		kvm_queue_exception(vcpu, DB_VECTOR);
>  }
>  
> +static void nested_svm_failed_vmrun(struct vcpu_svm *svm, struct vmcb *vmcb12)

I don't love the name.  "fail" has very specific meaning in VMX for VMLAUNCH and
VMRESUME, as VM-Fail is not a VM-Exit, e.g. doesn't load host state from the VMCS.

I also don't love that the name doesn't capture that this is synthesizing a #VMEXIT.
Maybe nested_svm_vmrun_error_vmexit()?  I suppose nested_svm_failed_vmrun_vmexit()
isn't too bad either, as that at least addresses my concerns about conflating it
with VMX's VM-Fail.

> +{
> +	WARN_ON(svm->vmcb == svm->nested.vmcb02.ptr);

WARN_ON_ONCE()

> +
> +	leave_guest_mode(vcpu);

Someone didn't test each patch.  "vcpu" doesn't exist until
"KVM: nSVM: Restrict mapping VMCB12 on nested VMRUN".  Just pass in @vcpu and
@vmcb12, i.e. don't pass @svm and then pull @vcpu back out.

> +	vmcb12->control.exit_code = SVM_EXIT_ERR;
> +	vmcb12->control.exit_code_hi = -1u;
> +	vmcb12->control.exit_info_1 = 0;
> +	vmcb12->control.exit_info_2 = 0;
> +	__nested_svm_vmexit(svm);
> +}

...

> @@ -1224,6 +1232,8 @@ void nested_svm_vmexit(struct vcpu_svm *svm)
>  	if (guest_cpu_cap_has(vcpu, X86_FEATURE_ERAPS))
>  		vmcb01->control.erap_ctl |= ERAP_CONTROL_CLEAR_RAP;
>  
> +	/* VMRUN failures before switching to VMCB02 are handled by nested_svm_failed_vmrun() */

Please don't add comments that just point elsewhere.  They inevitably become
stale, and they don't help the reader understand "why" any of this matters.

E.g. something like

	/*
	 * This helper is intended for use only when KVM synthesizing a #VMEXIT
	 * after a successful nested VMRUN.  All VMRUN consistency checks must
	 * be performed before loading guest state, and so should use the inner
	 * helper.
	 */