Re: [PATCH 3/8] KVM: SVM: Unwind "speculative" RIP advancement if INTn injection "fails"

From: Maxim Levitsky <mlevitsk@redhat.com>
To: Sean Christopherson <seanjc@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Maciej S . Szmigiero" <maciej.szmigiero@oracle.com>
Subject: Re: [PATCH 3/8] KVM: SVM: Unwind "speculative" RIP advancement if INTn injection "fails"
Date: Mon, 04 Apr 2022 13:03:21 +0300	[thread overview]
Message-ID: <2b26dd9569a0ae7a3d1fe1eab08010324d77e245.camel@redhat.com> (raw)
In-Reply-To: <20220402010903.727604-4-seanjc@google.com>

On Sat, 2022-04-02 at 01:08 +0000, Sean Christopherson wrote:
> Unwind the RIP advancement done by svm_queue_exception() when injecting
> an INT3 ultimately "fails" due to the CPU encountering a VM-Exit while
> vectoring the injected event, even if the exception reported by the CPU
> isn't the same event that was injected.  If vectoring INT3 encounters an
> exception, e.g. #NP, and vectoring the #NP encounters an intercepted
> exception, e.g. #PF when KVM is using shadow paging, then the #NP will
> be reported as the event that was in-progress.
> 
> Note, this is still imperfect, as it will get a false positive if the
> INT3 is cleanly injected, no VM-Exit occurs before the IRET from the INT3
> handler in the guest, the instruction following the INT3 generates an
> exception (directly or indirectly), _and_ vectoring that exception
> encounters an exception that is intercepted by KVM.  The false positives
> could theoretically be solved by further analyzing the vectoring event,
> e.g. by comparing the error code against the expected error code were an
> exception to occur when vectoring the original injected exception, but
> SVM without NRIPS is a complete disaster, trying to make it 100% correct
> is a waste of time.

Makes sense.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

> 
> Fixes: 66b7138f9136 ("KVM: SVM: Emulate nRIP feature when reinjecting INT3")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/svm/svm.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 2c86bd9176c6..30cef3b10838 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3699,6 +3699,18 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>  	vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
>  	type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
>  
> +	/*
> +	 * If NextRIP isn't enabled, KVM must manually advance RIP prior to
> +	 * injecting the soft exception/interrupt.  That advancement needs to
> +	 * be unwound if vectoring didn't complete.  Note, the _new_ event may
> +	 * not be the injected event, e.g. if KVM injected an INTn, the INTn
> +	 * hit a #NP in the guest, and the #NP encountered a #PF, the #NP will
> +	 * be the reported vectored event, but RIP still needs to be unwound.
> +	 */
> +	if (int3_injected && type == SVM_EXITINTINFO_TYPE_EXEPT &&
> +	   kvm_is_linear_rip(vcpu, svm->int3_rip))
> +		kvm_rip_write(vcpu, kvm_rip_read(vcpu) - int3_injected);
> +
>  	switch (type) {
>  	case SVM_EXITINTINFO_TYPE_NMI:
>  		vcpu->arch.nmi_injected = true;
> @@ -3715,13 +3727,9 @@ static void svm_complete_interrupts(struct kvm_vcpu *vcpu)
>  		 * but re-execute the instruction instead. Rewind RIP first
>  		 * if we emulated INT3 before.
>  		 */
> -		if (kvm_exception_is_soft(vector)) {
> -			if (vector == BP_VECTOR && int3_injected &&
> -			    kvm_is_linear_rip(vcpu, svm->int3_rip))
> -				kvm_rip_write(vcpu,
> -					      kvm_rip_read(vcpu) - int3_injected);
> +		if (kvm_exception_is_soft(vector))
>  			break;
> -		}
> +
>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>  			u32 err = svm->vmcb->control.exit_int_info_err;
>  			kvm_requeue_exception_e(vcpu, vector, err);