Re: [PATCH 2/2] KVM: SVM: Make stepping out of NMI handlers more robust

[Gleb Natapov <gleb@redhat.com>]

On Mon, Feb 15, 2010 at 07:17:18PM +0100, Jan Kiszka wrote:
> As there is no interception on AMD on the end of an NMI handler but only
> on its iret, we are forced to step out by setting TF in rflags. This can
> collide with host or guest using single-step mode, and it may leak the
> flags onto the guest stack if IRET triggers some exception.
The code is trying to handle the case where debugger used TF flags and we
want to single step from NMI handler simultaneously. Do you see problem with
that code? Uf yes may be it sill be much simpler to fix it? TF leakage is real,
but what problem it may cause? Note that your patch does not solve this problem
too. See the comment that you've deleted:
	/* Something prevents NMI from been injected. Single step over
	   possible problem (IRET or exception injection or interrupt
	   shadow) */
So the reason for single step is not necessary IRET, _any_ exception
is possible at this point.

> 
> This patch addresses the TF leakage by trapping all exceptions that may
> show up on IRET execution and cleaning up the state again before
> reinjecting the exception. Collisions with concurrent users are avoided
> by carefully checking for their presence and forwarding #DB exceptions
> whenever required.
This patch is scary to apply without test cases.

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/include/asm/kvm_host.h |    3 +
>  arch/x86/kvm/svm.c              |  112 +++++++++++++++++++++++++++++---------
>  2 files changed, 88 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index f9a2f66..cbae274 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -62,6 +62,7 @@
>  #define GP_VECTOR 13
>  #define PF_VECTOR 14
>  #define MF_VECTOR 16
> +#define AC_VECTOR 17
>  #define MC_VECTOR 18
>  
>  #define SELECTOR_TI_MASK (1 << 2)
> @@ -131,8 +132,10 @@ enum {
>  
>  #define KVM_NR_DB_REGS	4
>  
> +#define DR6_BP_MASK	0x0000000f
>  #define DR6_BD		(1 << 13)
>  #define DR6_BS		(1 << 14)
> +#define DR6_BT		(1 << 15)
>  #define DR6_FIXED_1	0xffff0ff0
>  #define DR6_VOLATILE	0x0000e00f
>  
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f63f1db..672f394 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -109,7 +109,8 @@ struct vcpu_svm {
>  
>  	struct nested_state nested;
>  
> -	bool nmi_singlestep;
> +	bool iret_singlestep;
> +	u64 iret_rflags;
>  
>  	bool int3_injected;
>  };
> @@ -1084,15 +1085,21 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
>  
>  }
>  
> +#define IRET_EXCEPTIONS (1 << NP_VECTOR) | (1 << SS_VECTOR) | \
> +			(1 << GP_VECTOR) | (1 << PF_VECTOR) | (1 << AC_VECTOR)
> +
>  static void update_db_intercept(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
>  	svm->vmcb->control.intercept_exceptions &=
> -		~((1 << DB_VECTOR) | (1 << BP_VECTOR));
> +		~((1 << DB_VECTOR) | (1 << BP_VECTOR) | IRET_EXCEPTIONS);
> +	if (!npt_enabled)
> +		svm->vmcb->control.intercept_exceptions |= 1 << PF_VECTOR;
>  
> -	if (svm->nmi_singlestep)
> -		svm->vmcb->control.intercept_exceptions |= (1 << DB_VECTOR);
> +	if (svm->iret_singlestep)
> +		svm->vmcb->control.intercept_exceptions |=
> +			(1 << DB_VECTOR) | IRET_EXCEPTIONS;
>  
>  	if (vcpu->guest_debug & KVM_GUESTDBG_ENABLE) {
>  		if (vcpu->guest_debug &
> @@ -1210,11 +1217,45 @@ static int svm_set_dr(struct kvm_vcpu *vcpu, int dr, unsigned long value)
>  	return EMULATE_DONE;
>  }
>  
> +static void reset_iret_singlestep(struct vcpu_svm * svm)
> +{
> +	svm->iret_singlestep = false;
> +	svm->vmcb->save.rflags = svm->iret_rflags;
> +	update_db_intercept(&svm->vcpu);
> +
> +	/* We did not yet leave the NMI handler. */
> +	svm->vcpu.arch.hflags |= HF_NMI_MASK;
> +}
> +
> +static int fault_on_iret(struct vcpu_svm *svm)
> +{
> +	u32 exit_code = svm->vmcb->control.exit_code;
> +
> +	BUG_ON(!svm->iret_singlestep);
> +
> +	reset_iret_singlestep(svm);
> +
> +	/*
> +	 * Requeue the exception IRET triggered and let the guest handle it.
> +	 * Note that all of them come with an error code.
> +	 */
> +	kvm_queue_exception_e(&svm->vcpu, exit_code - SVM_EXIT_EXCP_BASE,
> +			      svm->vmcb->control.exit_int_info_err);
> +	return 1;
> +}
> +
>  static int pf_interception(struct vcpu_svm *svm)
>  {
>  	u64 fault_address;
>  	u32 error_code;
>  
> +	if (svm->iret_singlestep) {
> +		if (npt_enabled)
> +			return fault_on_iret(svm);
> +
> +		reset_iret_singlestep(svm);
> +	}
> +
>  	fault_address  = svm->vmcb->control.exit_info_2;
>  	error_code = svm->vmcb->control.exit_info_1;
>  
> @@ -1227,32 +1268,43 @@ static int pf_interception(struct vcpu_svm *svm)
>  static int db_interception(struct vcpu_svm *svm)
>  {
>  	struct kvm_run *kvm_run = svm->vcpu.run;
> +	struct vmcb_save_area *save = &svm->vmcb->save;
>  
> -	if (!(svm->vcpu.guest_debug &
> -	      (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)) &&
> -		!svm->nmi_singlestep) {
> -		kvm_queue_exception(&svm->vcpu, DB_VECTOR);
> -		return 1;
> -	}
> +	if (svm->iret_singlestep) {
> +		if (!(save->dr6 & DR6_BS)) {
> +			 /* Not yet the step we are waiting for. */
> +			reset_iret_singlestep(svm);
> +			goto check_guest_debug;
> +		}
>  
> -	if (svm->nmi_singlestep) {
> -		svm->nmi_singlestep = false;
> -		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP))
> -			svm->vmcb->save.rflags &=
> -				~(X86_EFLAGS_TF | X86_EFLAGS_RF);
> +		svm->iret_singlestep = false;
>  		update_db_intercept(&svm->vcpu);
> +
> +		if (svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP)
> +			goto debugger_exit;
> +
> +		/*
> +		 * Check if there is some other reason in DR6 or if the guest
> +		 * is in single-step mode as well. If not, we are done.
> +		 */
> +		if (!(save->dr6 & (DR6_BP_MASK | DR6_BD | DR6_BT)) &&
> +		    (save->rflags & (X86_EFLAGS_TF | X86_EFLAGS_RF)) !=
> +		    X86_EFLAGS_TF)
> +			return 1;
>  	}
>  
> -	if (svm->vcpu.guest_debug &
> -	    (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP)){
> -		kvm_run->exit_reason = KVM_EXIT_DEBUG;
> -		kvm_run->debug.arch.pc =
> -			svm->vmcb->save.cs.base + svm->vmcb->save.rip;
> -		kvm_run->debug.arch.exception = DB_VECTOR;
> -		return 0;
> +check_guest_debug:
> +	if (!(svm->vcpu.guest_debug &
> +	      (KVM_GUESTDBG_SINGLESTEP | KVM_GUESTDBG_USE_HW_BP))) {
> +		kvm_queue_exception(&svm->vcpu, DB_VECTOR);
> +		return 1;
>  	}
>  
> -	return 1;
> +debugger_exit:
> +	kvm_run->exit_reason = KVM_EXIT_DEBUG;
> +	kvm_run->debug.arch.pc = svm->vmcb->save.cs.base + svm->vmcb->save.rip;
> +	kvm_run->debug.arch.exception = DB_VECTOR;
> +	return 0;
>  }
>  
>  static int bp_interception(struct vcpu_svm *svm)
> @@ -2367,6 +2419,10 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm) = {
>  	[SVM_EXIT_EXCP_BASE + PF_VECTOR] 	= pf_interception,
>  	[SVM_EXIT_EXCP_BASE + NM_VECTOR] 	= nm_interception,
>  	[SVM_EXIT_EXCP_BASE + MC_VECTOR] 	= mc_interception,
> +	[SVM_EXIT_EXCP_BASE + NP_VECTOR] 	= fault_on_iret,
> +	[SVM_EXIT_EXCP_BASE + SS_VECTOR] 	= fault_on_iret,
> +	[SVM_EXIT_EXCP_BASE + GP_VECTOR] 	= fault_on_iret,
> +	[SVM_EXIT_EXCP_BASE + AC_VECTOR] 	= fault_on_iret,
>  	[SVM_EXIT_INTR] 			= intr_interception,
>  	[SVM_EXIT_NMI]				= nmi_interception,
>  	[SVM_EXIT_SMI]				= nop_on_interception,
> @@ -2598,10 +2654,12 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
>  	    == HF_NMI_MASK)
>  		return; /* IRET will cause a vm exit */
>  
> -	/* Something prevents NMI from been injected. Single step over
> -	   possible problem (IRET or exception injection or interrupt
> -	   shadow) */
> -	svm->nmi_singlestep = true;
> +	/*
> +	 * Step over IRET. We will try to inject again after returning from
> +	 * the NMI handler.
> +	 */
> +	svm->iret_singlestep = true;
> +	svm->iret_rflags = svm->vmcb->save.rflags;
>  	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
>  	update_db_intercept(vcpu);
>  }
> -- 
> 1.6.0.2

--
			Gleb.