linux-kernel - Re: [PATCH v2 11/13] KVM: nSVM: Simplify nested_svm

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] [day] [month] [year] [list]

Message-ID: <ywd3yhhdpz3k5rc3rg3dpt7sq73kyvjt5d5tuzw7id75y25l5a@r5hvfviipfn3>
Date: Mon, 15 Dec 2025 18:34:56 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Jim Mattson <jmattson@...gle.com>, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 11/13] KVM: nSVM: Simplify nested_svm_vmrun()

On Tue, Dec 09, 2025 at 08:11:41AM -0800, Sean Christopherson wrote:
> On Mon, Nov 10, 2025, Yosry Ahmed wrote:
> > Call nested_svm_merge_msrpm() from enter_svm_guest_mode() if called from
> > the VMRUN path, instead of making the call in nested_svm_vmrun(). This
> > simplifies the flow of nested_svm_vmrun() and removes all jumps to
> > cleanup labels.
> > 
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> > ---
> >  arch/x86/kvm/svm/nested.c | 28 +++++++++++++---------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index a48668c36a191..89830380cebc5 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -1020,6 +1020,9 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa, bool from_vmrun)
> >  
> >  	nested_svm_hv_update_vm_vp_ids(vcpu);
> >  
> > +	if (from_vmrun && !nested_svm_merge_msrpm(vcpu))
> 
> This is silly, just do:
> 
> 	if (enter_svm_guest_mode(vcpu, vmcb12_gpa, true) ||
> 	    nested_svm_merge_msrpm(vcpu)) {
> 		svm->nested.nested_run_pending = 0;
> 		svm->nmi_l1_to_l2 = false;
> 		svm->soft_int_injected = false;
> 
> 		svm->vmcb->control.exit_code    = SVM_EXIT_ERR;
> 		svm->vmcb->control.exit_code_hi = -1u;
> 		svm->vmcb->control.exit_info_1  = 0;
> 		svm->vmcb->control.exit_info_2  = 0;
> 
> 		nested_svm_vmexit(svm);
> 	}
> 
> > +		return -1;
> 
> Please stop returning -1, use a proper -errno.
> 
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1105,23 +1108,18 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
> >  
> >  	svm->nested.nested_run_pending = 1;
> >  
> > -	if (enter_svm_guest_mode(vcpu, vmcb12_gpa, true))
> > -		goto out_exit_err;
> > -
> > -	if (nested_svm_merge_msrpm(vcpu))
> > -		return ret;
> > -
> > -out_exit_err:
> > -	svm->nested.nested_run_pending = 0;
> > -	svm->nmi_l1_to_l2 = false;
> > -	svm->soft_int_injected = false;
> > +	if (enter_svm_guest_mode(vcpu, vmcb12_gpa, true)) {
> > +		svm->nested.nested_run_pending = 0;
> > +		svm->nmi_l1_to_l2 = false;
> > +		svm->soft_int_injected = false;
> >  
> > -	svm->vmcb->control.exit_code    = SVM_EXIT_ERR;
> > -	svm->vmcb->control.exit_code_hi = 0;
> > -	svm->vmcb->control.exit_info_1  = 0;
> > -	svm->vmcb->control.exit_info_2  = 0;
> > +		svm->vmcb->control.exit_code    = SVM_EXIT_ERR;
> > +		svm->vmcb->control.exit_code_hi = 0;
> > +		svm->vmcb->control.exit_info_1  = 0;
> > +		svm->vmcb->control.exit_info_2  = 0;
> >  
> > -	nested_svm_vmexit(svm);
> > +		nested_svm_vmexit(svm);
> 
> Note, there's a pre-existing bug in nested_svm_vmexit().  Lovely, and it's a
> user-triggerable WARN_ON() (and not even a WARN_ON_ONCE() at that).
> 
> If nested_svm_vmexit() fails to map vmcb12, it (unbelievably stupidly) injects a
> #GP and hopes for the best.  Oh FFS, it also has the asinine -EINVAL "logic".
> Anyways, it injects #GP (maybe), and bails early, which leaves
> KVM_REQ_GET_NESTED_STATE_PAGES set.  KVM will then process that on the next
> vcpu_enter_guest() and trip the WARN_ON() in svm_get_nested_state_pages().

FWIW, I don't think there will be a warning because when
nested_svm_vmexit() fails to map vmcb12 it also fails to leave guest
mode, so the WARN_ON() should not fire.

I still agree this is a bug and will include a fix/cleanup in the next
version that I will send out shortly.