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: <rp3tkueohngrri3fjyibmnzjzfqpyxv73j7i6hvdtrnobuetfd@xilmrwwre6bd>
Date: Fri, 6 Feb 2026 15:40:50 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
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, Feb 05, 2026 at 04:47:59PM -0800, Sean Christopherson wrote:
> 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.

Good point, I think nested_svm_vmrun_error_vmexit() is fine.

> 
> > +{
> > +	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

I plead the 5th.

> "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.
> 	 */ 

Will use that, but I will replace "this helper" and "inner helper" with
nested_svm_vmexit() and __nested_svm_vmexit().