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

Message-ID: <aTysvq7v7Zp0_4xz@google.com>
Date: Fri, 12 Dec 2025 16:01:02 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Yosry Ahmed <yosry.ahmed@...ux.dev>
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 Thu, Dec 11, 2025, Yosry Ahmed wrote:
> On Thu, Dec 11, 2025 at 07:25:21PM +0000, Yosry Ahmed wrote:
> > 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);
> > > 	}
> > 
> > Actually, if we go with the approach of making all VMRUN failures
> > happen before preparing the VMCB02 (as discussed in the other thread),
> > then we will want to call nested_svm_merge_msrpm() from within
> > enter_svm_guest_mode().
> 
> We can also just call nested_svm_merge_msrpm() before
> enter_svm_guest_mode(), which seems to work. 

That's likely unsafe, nested_vmcb_check_controls() checks fields that are consumed
by nested_svm_merge_msrpm().

> > Otherwise, we either have a separate failure path for
> > nested_svm_merge_msrpm(), or we make all VMRUN failures happen after
> > preparing the VMCB02 and handled by nested_svm_vmexit().
> > 
> > I like having a separate exit path for VMRUN failures, and it makes more
> > sense to do the consistency checks on VMCB12 before preparing VMCB02.
> > But I understand if you prefer to keep things simple and move all
> > failures after VMCB02.
> > 
> > I already have it implemented with the separate VMRUN failure path, but
> > I don't wanna spam you with another series if you prefer it the other
> > way.

Spam away.