[PATCH 0/2] KVM: SVM improvements around INT3 and NMI

[PATCH 0/2] KVM: SVM improvements around INT3 and NMI

[Jan Kiszka]

Two patches that aim at improving some hairy SVM parts: The emulation of
INT3 reinjection on older processors without nRIP support (required for
proper guest debugging) and the infamous NMI handling.

Unfortunately, I do not have test cases for all scenarios involved.
Specifically the exception-during-IRET-from-NMI requires a hand-crafted
test that does not exist yet. However, patch 1 was successfully tested
on a nRIP-capable host by disabling that bit, and patch 2 survived
basic tests, including single-step out of NMI via guest debugging.

If the core idea of patch 2 - interception of all exceptions that the
problematic instruction causes - works, it may also be applied on INT3.
However, I'm reluctant to invest too much effort in this given that the
remaining open issues are very improbable to show up in practice.

Please check sceptically, I surely messed up some corner case.

Jan Kiszka (2):
  KVM: SVM: Emulate nRIP feature when reinjecting INT3
  KVM: SVM: Make stepping out of NMI handlers more robust

 arch/x86/include/asm/kvm_host.h |    3 +
 arch/x86/kvm/svm.c              |  166 ++++++++++++++++++++++++++++----------
 2 files changed, 125 insertions(+), 44 deletions(-)

[PATCH 1/2] KVM: SVM: Emulate nRIP feature when reinjecting INT3

[Jan Kiszka]

When in guest debugging mode, we have to reinject those #BP software
exceptions that are caused by guest-injected INT3. As older AMD
processors to not support the required nRIP VMCB field, try to emulate
it by moving RIP by one on injection. Fix it up again in case the
injection failed and we were able to catch this. This does not work for
unintercepted faults, but it is better than doing nothing.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/x86/kvm/svm.c |   54 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 52f78dd..f63f1db 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -46,6 +46,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_NRIP (1 << 3)
 #define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define NESTED_EXIT_HOST	0	/* Exit handled on host level */
@@ -109,6 +110,8 @@ struct vcpu_svm {
 	struct nested_state nested;
 
 	bool nmi_singlestep;
+
+	bool int3_injected;
 };
 
 /* enable NPT for AMD64 and X86 with PAE */
@@ -234,23 +237,6 @@ static void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	vcpu->arch.efer = efer;
 }
 
-static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
-				bool has_error_code, u32 error_code)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-
-	/* If we are within a nested VM we'd better #VMEXIT and let the
-	   guest handle the exception */
-	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
-		return;
-
-	svm->vmcb->control.event_inj = nr
-		| SVM_EVTINJ_VALID
-		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
-		| SVM_EVTINJ_TYPE_EXEPT;
-	svm->vmcb->control.event_inj_err = error_code;
-}
-
 static int is_external_interrupt(u32 info)
 {
 	info &= SVM_EVTINJ_TYPE_MASK | SVM_EVTINJ_VALID;
@@ -296,6 +282,36 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	svm_set_interrupt_shadow(vcpu, 0);
 }
 
+static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
+				bool has_error_code, u32 error_code)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	/* If we are within a nested VM we'd better #VMEXIT and let the
+	   guest handle the exception */
+	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
+		return;
+
+	if (nr == BP_VECTOR && !svm_has(SVM_FEATURE_NRIP)) {
+		/*
+		 * For guest debugging where we have to reinject #BP if some
+		 * INT3 is guest-owned:
+		 * Emulate nRIP by moving RIP one forward. Will fail if
+		 * injection raises a fault that is not intercepted. Still
+		 * better than failing in all cases.
+		 */
+		svm->next_rip = kvm_rip_read(&svm->vcpu) + 1;
+		skip_emulated_instruction(&svm->vcpu);
+		svm->int3_injected = true;
+	}
+
+	svm->vmcb->control.event_inj = nr
+		| SVM_EVTINJ_VALID
+		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
+		| SVM_EVTINJ_TYPE_EXEPT;
+	svm->vmcb->control.event_inj_err = error_code;
+}
+
 static int has_svm(void)
 {
 	const char *msg;
@@ -2653,6 +2669,9 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 		if (is_nested(svm))
 			break;
 		if (kvm_exception_is_soft(vector))
+			if (vector == BP_VECTOR && svm->int3_injected)
+				kvm_rip_write(&svm->vcpu,
+					      kvm_rip_read(&svm->vcpu) - 1);
 			break;
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
 			u32 err = svm->vmcb->control.exit_int_info_err;
@@ -2667,6 +2686,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	default:
 		break;
 	}
+	svm->int3_injected =false;
 }
 
 #ifdef CONFIG_X86_64
-- 
1.6.0.2

Re: [PATCH 1/2] KVM: SVM: Emulate nRIP feature when reinjecting INT3

[Gleb Natapov]

On Mon, Feb 15, 2010 at 07:17:17PM +0100, Jan Kiszka wrote:
> When in guest debugging mode, we have to reinject those #BP software
> exceptions that are caused by guest-injected INT3. As older AMD
> processors to not support the required nRIP VMCB field, try to emulate
> it by moving RIP by one on injection. Fix it up again in case the
> injection failed and we were able to catch this. This does not work for
> unintercepted faults, but it is better than doing nothing.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/x86/kvm/svm.c |   54 +++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 52f78dd..f63f1db 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -46,6 +46,7 @@ MODULE_LICENSE("GPL");
>  #define SVM_FEATURE_NPT  (1 << 0)
>  #define SVM_FEATURE_LBRV (1 << 1)
>  #define SVM_FEATURE_SVML (1 << 2)
> +#define SVM_FEATURE_NRIP (1 << 3)
>  #define SVM_FEATURE_PAUSE_FILTER (1 << 10)
>  
>  #define NESTED_EXIT_HOST	0	/* Exit handled on host level */
> @@ -109,6 +110,8 @@ struct vcpu_svm {
>  	struct nested_state nested;
>  
>  	bool nmi_singlestep;
> +
> +	bool int3_injected;
>  };
>  
>  /* enable NPT for AMD64 and X86 with PAE */
> @@ -234,23 +237,6 @@ static void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	vcpu->arch.efer = efer;
>  }
>  
> -static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
> -				bool has_error_code, u32 error_code)
> -{
> -	struct vcpu_svm *svm = to_svm(vcpu);
> -
> -	/* If we are within a nested VM we'd better #VMEXIT and let the
> -	   guest handle the exception */
> -	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
> -		return;
> -
> -	svm->vmcb->control.event_inj = nr
> -		| SVM_EVTINJ_VALID
> -		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
> -		| SVM_EVTINJ_TYPE_EXEPT;
> -	svm->vmcb->control.event_inj_err = error_code;
> -}
> -
>  static int is_external_interrupt(u32 info)
>  {
>  	info &= SVM_EVTINJ_TYPE_MASK | SVM_EVTINJ_VALID;
> @@ -296,6 +282,36 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	svm_set_interrupt_shadow(vcpu, 0);
>  }
>  
> +static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
> +				bool has_error_code, u32 error_code)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +
> +	/* If we are within a nested VM we'd better #VMEXIT and let the
> +	   guest handle the exception */
> +	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
> +		return;
> +
> +	if (nr == BP_VECTOR && !svm_has(SVM_FEATURE_NRIP)) {
> +		/*
> +		 * For guest debugging where we have to reinject #BP if some
> +		 * INT3 is guest-owned:
> +		 * Emulate nRIP by moving RIP one forward. Will fail if
> +		 * injection raises a fault that is not intercepted. Still
> +		 * better than failing in all cases.
> +		 */
> +		svm->next_rip = kvm_rip_read(&svm->vcpu) + 1;
> +		skip_emulated_instruction(&svm->vcpu);
if next_rip is zero skip_emulated_instruction() decodes instruction by
itself to properly calculate next rip so no need to guess instruction
length.

> +		svm->int3_injected = true;
> +	}
> +
> +	svm->vmcb->control.event_inj = nr
> +		| SVM_EVTINJ_VALID
> +		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
> +		| SVM_EVTINJ_TYPE_EXEPT;
> +	svm->vmcb->control.event_inj_err = error_code;
> +}
> +
>  static int has_svm(void)
>  {
>  	const char *msg;
> @@ -2653,6 +2669,9 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>  		if (is_nested(svm))
>  			break;
>  		if (kvm_exception_is_soft(vector))
> +			if (vector == BP_VECTOR && svm->int3_injected)
> +				kvm_rip_write(&svm->vcpu,
> +					      kvm_rip_read(&svm->vcpu) - 1);
You don't even check current rip? So if fault happens during unrelated #BP you move
rip backwards and restart.

>  			break;
>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>  			u32 err = svm->vmcb->control.exit_int_info_err;
> @@ -2667,6 +2686,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>  	default:
>  		break;
>  	}
> +	svm->int3_injected =false;
Looks like the wrong place to clear this. It should be cleared on every
exit, not only with valid vectoring info.

>  }
>  
>  #ifdef CONFIG_X86_64
> -- 
> 1.6.0.2

--
			Gleb.

Re: [PATCH 1/2] KVM: SVM: Emulate nRIP feature when reinjecting INT3

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 4731 bytes --]

Gleb Natapov wrote:
> On Mon, Feb 15, 2010 at 07:17:17PM +0100, Jan Kiszka wrote:
>> When in guest debugging mode, we have to reinject those #BP software
>> exceptions that are caused by guest-injected INT3. As older AMD
>> processors to not support the required nRIP VMCB field, try to emulate
>> it by moving RIP by one on injection. Fix it up again in case the
>> injection failed and we were able to catch this. This does not work for
>> unintercepted faults, but it is better than doing nothing.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  arch/x86/kvm/svm.c |   54 +++++++++++++++++++++++++++++++++++----------------
>>  1 files changed, 37 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 52f78dd..f63f1db 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -46,6 +46,7 @@ MODULE_LICENSE("GPL");
>>  #define SVM_FEATURE_NPT  (1 << 0)
>>  #define SVM_FEATURE_LBRV (1 << 1)
>>  #define SVM_FEATURE_SVML (1 << 2)
>> +#define SVM_FEATURE_NRIP (1 << 3)
>>  #define SVM_FEATURE_PAUSE_FILTER (1 << 10)
>>  
>>  #define NESTED_EXIT_HOST	0	/* Exit handled on host level */
>> @@ -109,6 +110,8 @@ struct vcpu_svm {
>>  	struct nested_state nested;
>>  
>>  	bool nmi_singlestep;
>> +
>> +	bool int3_injected;
>>  };
>>  
>>  /* enable NPT for AMD64 and X86 with PAE */
>> @@ -234,23 +237,6 @@ static void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>>  	vcpu->arch.efer = efer;
>>  }
>>  
>> -static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>> -				bool has_error_code, u32 error_code)
>> -{
>> -	struct vcpu_svm *svm = to_svm(vcpu);
>> -
>> -	/* If we are within a nested VM we'd better #VMEXIT and let the
>> -	   guest handle the exception */
>> -	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
>> -		return;
>> -
>> -	svm->vmcb->control.event_inj = nr
>> -		| SVM_EVTINJ_VALID
>> -		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
>> -		| SVM_EVTINJ_TYPE_EXEPT;
>> -	svm->vmcb->control.event_inj_err = error_code;
>> -}
>> -
>>  static int is_external_interrupt(u32 info)
>>  {
>>  	info &= SVM_EVTINJ_TYPE_MASK | SVM_EVTINJ_VALID;
>> @@ -296,6 +282,36 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>>  	svm_set_interrupt_shadow(vcpu, 0);
>>  }
>>  
>> +static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
>> +				bool has_error_code, u32 error_code)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +
>> +	/* If we are within a nested VM we'd better #VMEXIT and let the
>> +	   guest handle the exception */
>> +	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
>> +		return;
>> +
>> +	if (nr == BP_VECTOR && !svm_has(SVM_FEATURE_NRIP)) {
>> +		/*
>> +		 * For guest debugging where we have to reinject #BP if some
>> +		 * INT3 is guest-owned:
>> +		 * Emulate nRIP by moving RIP one forward. Will fail if
>> +		 * injection raises a fault that is not intercepted. Still
>> +		 * better than failing in all cases.
>> +		 */
>> +		svm->next_rip = kvm_rip_read(&svm->vcpu) + 1;
>> +		skip_emulated_instruction(&svm->vcpu);
> if next_rip is zero skip_emulated_instruction() decodes instruction by
> itself to properly calculate next rip so no need to guess instruction
> length.

Just copied what all the instruction emulations do here. Can change, though.

> 
>> +		svm->int3_injected = true;
>> +	}
>> +
>> +	svm->vmcb->control.event_inj = nr
>> +		| SVM_EVTINJ_VALID
>> +		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
>> +		| SVM_EVTINJ_TYPE_EXEPT;
>> +	svm->vmcb->control.event_inj_err = error_code;
>> +}
>> +
>>  static int has_svm(void)
>>  {
>>  	const char *msg;
>> @@ -2653,6 +2669,9 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>>  		if (is_nested(svm))
>>  			break;
>>  		if (kvm_exception_is_soft(vector))
>> +			if (vector == BP_VECTOR && svm->int3_injected)
>> +				kvm_rip_write(&svm->vcpu,
>> +					      kvm_rip_read(&svm->vcpu) - 1);
> You don't even check current rip? So if fault happens during unrelated #BP you move
> rip backwards and restart.

Yep, true. Will fix.

> 
>>  			break;
>>  		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
>>  			u32 err = svm->vmcb->control.exit_int_info_err;
>> @@ -2667,6 +2686,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
>>  	default:
>>  		break;
>>  	}
>> +	svm->int3_injected =false;
> Looks like the wrong place to clear this. It should be cleared on every
> exit, not only with valid vectoring info.

Right.

> 
>>  }
>>  
>>  #ifdef CONFIG_X86_64
>> -- 
>> 1.6.0.2
> 
> --
> 			Gleb.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[PATCH v2 1/2] KVM: SVM: Emulate nRIP feature when reinjecting INT3

[Jan Kiszka]

When in guest debugging mode, we have to reinject those #BP software
exceptions that are caused by guest-injected INT3. As older AMD
processors to not support the required nRIP VMCB field, try to emulate
it by moving RIP by one on injection. Fix it up again in case the
injection failed and we were able to catch this. This does not work for
unintercepted faults, but it is better than doing nothing.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - let skip_emulated_instruction find the instruction length
 - only rewind rip of the right INT3 failed
 - clear itn3_injected also if exitintinfo is invalid

 arch/x86/kvm/svm.c |   72 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 84c838d..9b040bc 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -46,6 +46,7 @@ MODULE_LICENSE("GPL");
 #define SVM_FEATURE_NPT  (1 << 0)
 #define SVM_FEATURE_LBRV (1 << 1)
 #define SVM_FEATURE_SVML (1 << 2)
+#define SVM_FEATURE_NRIP (1 << 3)
 #define SVM_FEATURE_PAUSE_FILTER (1 << 10)
 
 #define NESTED_EXIT_HOST	0	/* Exit handled on host level */
@@ -109,6 +110,10 @@ struct vcpu_svm {
 	struct nested_state nested;
 
 	bool nmi_singlestep;
+
+	unsigned int3_injected;
+	u16 int3_cs;
+	u64 int3_rip;
 };
 
 /* enable NPT for AMD64 and X86 with PAE */
@@ -234,23 +239,6 @@ static void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer)
 	vcpu->arch.efer = efer;
 }
 
-static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
-				bool has_error_code, u32 error_code)
-{
-	struct vcpu_svm *svm = to_svm(vcpu);
-
-	/* If we are within a nested VM we'd better #VMEXIT and let the
-	   guest handle the exception */
-	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
-		return;
-
-	svm->vmcb->control.event_inj = nr
-		| SVM_EVTINJ_VALID
-		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
-		| SVM_EVTINJ_TYPE_EXEPT;
-	svm->vmcb->control.event_inj_err = error_code;
-}
-
 static int is_external_interrupt(u32 info)
 {
 	info &= SVM_EVTINJ_TYPE_MASK | SVM_EVTINJ_VALID;
@@ -296,6 +284,39 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
 	kvm_rip_write(vcpu, svm->next_rip);
 }
 
+static void svm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr,
+				bool has_error_code, u32 error_code)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+
+	/* If we are within a nested VM we'd better #VMEXIT and let the
+	   guest handle the exception */
+	if (nested_svm_check_exception(svm, nr, has_error_code, error_code))
+		return;
+
+	if (nr == BP_VECTOR && !svm_has(SVM_FEATURE_NRIP)) {
+		u64 old_rip = kvm_rip_read(&svm->vcpu);
+
+		/*
+		 * For guest debugging where we have to reinject #BP if some
+		 * INT3 is guest-owned:
+		 * Emulate nRIP by moving RIP forward. Will fail if injection
+		 * raises a fault that is not intercepted. Still better than
+		 * failing in all cases.
+		 */
+		skip_emulated_instruction(&svm->vcpu);
+		svm->int3_cs = svm->vmcb->save.cs.selector;
+		svm->int3_rip = kvm_rip_read(&svm->vcpu);
+		svm->int3_injected = svm->int3_rip - old_rip;
+	}
+
+	svm->vmcb->control.event_inj = nr
+		| SVM_EVTINJ_VALID
+		| (has_error_code ? SVM_EVTINJ_VALID_ERR : 0)
+		| SVM_EVTINJ_TYPE_EXEPT;
+	svm->vmcb->control.event_inj_err = error_code;
+}
+
 static int has_svm(void)
 {
 	const char *msg;
@@ -2637,8 +2658,10 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	kvm_clear_exception_queue(&svm->vcpu);
 	kvm_clear_interrupt_queue(&svm->vcpu);
 
-	if (!(exitintinfo & SVM_EXITINTINFO_VALID))
+	if (!(exitintinfo & SVM_EXITINTINFO_VALID)) {
+		svm->int3_injected = 0;
 		return;
+	}
 
 	vector = exitintinfo & SVM_EXITINTINFO_VEC_MASK;
 	type = exitintinfo & SVM_EXITINTINFO_TYPE_MASK;
@@ -2648,12 +2671,20 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 		svm->vcpu.arch.nmi_injected = true;
 		break;
 	case SVM_EXITINTINFO_TYPE_EXEPT:
-		/* In case of software exception do not reinject an exception
-		   vector, but re-execute and instruction instead */
 		if (is_nested(svm))
 			break;
 		if (kvm_exception_is_soft(vector))
+			if (vector == BP_VECTOR && svm->int3_injected &&
+			    svm->vmcb->save.cs.selector == svm->int3_cs &&
+			    kvm_rip_read(&svm->vcpu) == svm->int3_rip)
+				kvm_rip_write(&svm->vcpu,
+					      kvm_rip_read(&svm->vcpu) -
+					      svm->int3_injected);
 			break;
+		/*
+		 * In case of other software exceptions, do not reinject the
+		 * vector, but re-execute the instruction instead.
+		 */
 		if (exitintinfo & SVM_EXITINTINFO_VALID_ERR) {
 			u32 err = svm->vmcb->control.exit_int_info_err;
 			kvm_queue_exception_e(&svm->vcpu, vector, err);
@@ -2667,6 +2698,7 @@ static void svm_complete_interrupts(struct vcpu_svm *svm)
 	default:
 		break;
 	}
+	svm->int3_injected = 0;
 }
 
 #ifdef CONFIG_X86_64

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

[Jan Kiszka]

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.

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.

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

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

[Gleb Natapov]

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.

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

[Jan Kiszka]

Gleb Natapov wrote:
> 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.

That is exactly what my code tries to avoid: Exceptions are all (famous
last word) caught, and single-stepping is disabled until that is
resolved. So no more leakage, and only IRET remains as reason here (thus
my deletion).

> 
>> 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.

Do you have one for the exception case? The others should have been
covered manually here, but I agree we should write a unit test, also for
the sake of VMX.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Gleb Natapov]

On Tue, Feb 16, 2010 at 10:14:56AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > 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.
> 
> That is exactly what my code tries to avoid: Exceptions are all (famous
> last word) caught, and single-stepping is disabled until that is
> resolved. So no more leakage, and only IRET remains as reason here (thus
> my deletion).
> 
I don't understand why only IRET remains as a reason here? Code will get
there if interrupt shadow is in effect too and then next instruction may
generate any exception not only those that IRET generates.

Also you haven't answered what is the problem with current code (except
TF leakage) and why TF leakage is so important. BTW are you sure that TF
leakage actually happens? I see in Intel SDM:

 The processor clears the TF flag before calling the exception handler.

> > 
> >> 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.
> 
> Do you have one for the exception case? The others should have been
I have one that check exception during nmi delivery and one that check
that nested nmi is delivered after iret. I don't have one that fault
during iret.

> covered manually here, but I agree we should write a unit test, also for
> the sake of VMX.
> 

--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Tue, Feb 16, 2010 at 10:14:56AM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> 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.
>> That is exactly what my code tries to avoid: Exceptions are all (famous
>> last word) caught, and single-stepping is disabled until that is
>> resolved. So no more leakage, and only IRET remains as reason here (thus
>> my deletion).
>>
> I don't understand why only IRET remains as a reason here? Code will get
> there if interrupt shadow is in effect too and then next instruction may
> generate any exception not only those that IRET generates.

OK, so the faults raised by the instruction under the interrupt shadow
can still cause troubles. Guess we have to live with it unless we what
to trap all exceptions that instructions can raise. Will adjust the comment.

> 
> Also you haven't answered what is the problem with current code (except
> TF leakage) and why TF leakage is so important. BTW are you sure that TF
> leakage actually happens? I see in Intel SDM:
> 
>  The processor clears the TF flag before calling the exception handler.

Does it clear it _for_ the exception handler or also in rflags pushed on
the stack?

Besides this, proper #DB forwarding to the guest was missing.

> 
>>>> 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.
>> Do you have one for the exception case? The others should have been
> I have one that check exception during nmi delivery and one that check
> that nested nmi is delivered after iret. I don't have one that fault
> during iret.
> 
>> covered manually here, but I agree we should write a unit test, also for
>> the sake of VMX.
>>
> 
> --
> 			Gleb.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Gleb Natapov]

On Tue, Feb 16, 2010 at 10:45:15AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Tue, Feb 16, 2010 at 10:14:56AM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> 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.
> >> That is exactly what my code tries to avoid: Exceptions are all (famous
> >> last word) caught, and single-stepping is disabled until that is
> >> resolved. So no more leakage, and only IRET remains as reason here (thus
> >> my deletion).
> >>
> > I don't understand why only IRET remains as a reason here? Code will get
> > there if interrupt shadow is in effect too and then next instruction may
> > generate any exception not only those that IRET generates.
> 
> OK, so the faults raised by the instruction under the interrupt shadow
> can still cause troubles. Guess we have to live with it unless we what
> to trap all exceptions that instructions can raise. Will adjust the comment.
> 
I don't see the point to complicate code significantly to fix it only
partially.

> > 
> > Also you haven't answered what is the problem with current code (except
> > TF leakage) and why TF leakage is so important. BTW are you sure that TF
> > leakage actually happens? I see in Intel SDM:
> > 
> >  The processor clears the TF flag before calling the exception handler.
> 
> Does it clear it _for_ the exception handler or also in rflags pushed on
> the stack?
Have no idea. Looking for relevant info in SDM.

> 
> Besides this, proper #DB forwarding to the guest was missing.
During NMI injection? How to reproduce?

--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Tue, Feb 16, 2010 at 10:45:15AM +0100, Jan Kiszka wrote:
>> Gleb Natapov wrote:
>>> On Tue, Feb 16, 2010 at 10:14:56AM +0100, Jan Kiszka wrote:
>>>> Gleb Natapov wrote:
>>>>> 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.
>>>> That is exactly what my code tries to avoid: Exceptions are all (famous
>>>> last word) caught, and single-stepping is disabled until that is
>>>> resolved. So no more leakage, and only IRET remains as reason here (thus
>>>> my deletion).
>>>>
>>> I don't understand why only IRET remains as a reason here? Code will get
>>> there if interrupt shadow is in effect too and then next instruction may
>>> generate any exception not only those that IRET generates.
>> OK, so the faults raised by the instruction under the interrupt shadow
>> can still cause troubles. Guess we have to live with it unless we what
>> to trap all exceptions that instructions can raise. Will adjust the comment.
>>
> I don't see the point to complicate code significantly to fix it only
> partially.

Maybe we can even fix it completely, just need to move some code around
and add checks to those few existing exception handlers. Will think
about it.

> 
>>> Also you haven't answered what is the problem with current code (except
>>> TF leakage) and why TF leakage is so important. BTW are you sure that TF
>>> leakage actually happens? I see in Intel SDM:
>>>
>>>  The processor clears the TF flag before calling the exception handler.
>> Does it clear it _for_ the exception handler or also in rflags pushed on
>> the stack?
> Have no idea. Looking for relevant info in SDM.
> 
>> Besides this, proper #DB forwarding to the guest was missing.
> During NMI injection? How to reproduce?

Inject, e.g., an NMI over code with TF set. A bit harder is placing a
guest HW breakpoint at the spot the NMI handler returns to.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Gleb Natapov]

On Tue, Feb 16, 2010 at 11:05:55AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Tue, Feb 16, 2010 at 10:45:15AM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Tue, Feb 16, 2010 at 10:14:56AM +0100, Jan Kiszka wrote:
> >>>> Gleb Natapov wrote:
> >>>>> 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.
> >>>> That is exactly what my code tries to avoid: Exceptions are all (famous
> >>>> last word) caught, and single-stepping is disabled until that is
> >>>> resolved. So no more leakage, and only IRET remains as reason here (thus
> >>>> my deletion).
> >>>>
> >>> I don't understand why only IRET remains as a reason here? Code will get
> >>> there if interrupt shadow is in effect too and then next instruction may
> >>> generate any exception not only those that IRET generates.
> >> OK, so the faults raised by the instruction under the interrupt shadow
> >> can still cause troubles. Guess we have to live with it unless we what
> >> to trap all exceptions that instructions can raise. Will adjust the comment.
> >>
> > I don't see the point to complicate code significantly to fix it only
> > partially.
> 
> Maybe we can even fix it completely, just need to move some code around
> and add checks to those few existing exception handlers. Will think
> about it.
> 
By catching all exceptions may be, but is it worth it?

> > 
> >>> Also you haven't answered what is the problem with current code (except
> >>> TF leakage) and why TF leakage is so important. BTW are you sure that TF
> >>> leakage actually happens? I see in Intel SDM:
> >>>
> >>>  The processor clears the TF flag before calling the exception handler.
> >> Does it clear it _for_ the exception handler or also in rflags pushed on
> >> the stack?
> > Have no idea. Looking for relevant info in SDM.
> > 
> >> Besides this, proper #DB forwarding to the guest was missing.
> > During NMI injection? How to reproduce?
> 
> Inject, e.g., an NMI over code with TF set. A bit harder is placing a
> guest HW breakpoint at the spot the NMI handler returns to.
> 
Will try to reproduce.

--
			Gleb.

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

[Gleb Natapov]

On Tue, Feb 16, 2010 at 12:08:58PM +0200, Gleb Natapov wrote:
> > > 
> > >> Besides this, proper #DB forwarding to the guest was missing.
> > > During NMI injection? How to reproduce?
> > 
> > Inject, e.g., an NMI over code with TF set. A bit harder is placing a
> > guest HW breakpoint at the spot the NMI handler returns to.
> > 
> Will try to reproduce.
> 
How can I make gdb to run debugged process with TF set? Is this patch
fixes it:


diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 52f78dd..b85b200 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -109,6 +109,7 @@ struct vcpu_svm {
 	struct nested_state nested;
 
 	bool nmi_singlestep;
+	bool nmi_singlestep_tf;
 };
 
 /* enable NPT for AMD64 and X86 with PAE */
@@ -1221,9 +1222,14 @@ static int db_interception(struct vcpu_svm *svm)
 
 	if (svm->nmi_singlestep) {
 		svm->nmi_singlestep = false;
-		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP))
+		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
 			svm->vmcb->save.rflags &=
 				~(X86_EFLAGS_TF | X86_EFLAGS_RF);
+			if (svm->nmi_singlestep_tf) {
+				svm->vmcb->save.rflags |= X86_EFLAGS_TF;
+				kvm_queue_exception(&svm->vcpu, DB_VECTOR);
+			}
+		}
 		update_db_intercept(&svm->vcpu);
 	}
 
@@ -2586,6 +2592,7 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
 	   possible problem (IRET or exception injection or interrupt
 	   shadow) */
 	svm->nmi_singlestep = true;
+	svm->nmi_singlestep_tf = (svm->vmcb->save.rflags | X86_EFLAGS_TF);
 	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
 	update_db_intercept(vcpu);
 }
--
			Gleb.

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

[Jan Kiszka]

Gleb Natapov wrote:
> On Tue, Feb 16, 2010 at 12:08:58PM +0200, Gleb Natapov wrote:
>>>>> Besides this, proper #DB forwarding to the guest was missing.
>>>> During NMI injection? How to reproduce?
>>> Inject, e.g., an NMI over code with TF set. A bit harder is placing a
>>> guest HW breakpoint at the spot the NMI handler returns to.
>>>
>> Will try to reproduce.
>>
> How can I make gdb to run debugged process with TF set? Is this patch
> fixes it:
> 
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 52f78dd..b85b200 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -109,6 +109,7 @@ struct vcpu_svm {
>  	struct nested_state nested;
>  
>  	bool nmi_singlestep;
> +	bool nmi_singlestep_tf;
>  };
>  
>  /* enable NPT for AMD64 and X86 with PAE */
> @@ -1221,9 +1222,14 @@ static int db_interception(struct vcpu_svm *svm)
>  
>  	if (svm->nmi_singlestep) {
>  		svm->nmi_singlestep = false;
> -		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP))
> +		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
>  			svm->vmcb->save.rflags &=
>  				~(X86_EFLAGS_TF | X86_EFLAGS_RF);
> +			if (svm->nmi_singlestep_tf) {
> +				svm->vmcb->save.rflags |= X86_EFLAGS_TF;
> +				kvm_queue_exception(&svm->vcpu, DB_VECTOR);
> +			}
> +		}
>  		update_db_intercept(&svm->vcpu);
>  	}
>  
> @@ -2586,6 +2592,7 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
>  	   possible problem (IRET or exception injection or interrupt
>  	   shadow) */
>  	svm->nmi_singlestep = true;
> +	svm->nmi_singlestep_tf = (svm->vmcb->save.rflags | X86_EFLAGS_TF);
>  	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
>  	update_db_intercept(vcpu);
>  }

That's closer. However, I've a version here that restores TF&RF only if
you did not execute an IRET but stepped over the shadow (which is still
not correct either, e.g. when stepping popf). I will break up my patch
into parts that fix the issues separately so that we can decide what to
merge.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Gleb Natapov]

On Wed, Feb 17, 2010 at 08:16:45PM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Tue, Feb 16, 2010 at 12:08:58PM +0200, Gleb Natapov wrote:
> >>>>> Besides this, proper #DB forwarding to the guest was missing.
> >>>> During NMI injection? How to reproduce?
> >>> Inject, e.g., an NMI over code with TF set. A bit harder is placing a
> >>> guest HW breakpoint at the spot the NMI handler returns to.
> >>>
> >> Will try to reproduce.
> >>
> > How can I make gdb to run debugged process with TF set? Is this patch
> > fixes it:
> > 
> > 
> > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> > index 52f78dd..b85b200 100644
> > --- a/arch/x86/kvm/svm.c
> > +++ b/arch/x86/kvm/svm.c
> > @@ -109,6 +109,7 @@ struct vcpu_svm {
> >  	struct nested_state nested;
> >  
> >  	bool nmi_singlestep;
> > +	bool nmi_singlestep_tf;
> >  };
> >  
> >  /* enable NPT for AMD64 and X86 with PAE */
> > @@ -1221,9 +1222,14 @@ static int db_interception(struct vcpu_svm *svm)
> >  
> >  	if (svm->nmi_singlestep) {
> >  		svm->nmi_singlestep = false;
> > -		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP))
> > +		if (!(svm->vcpu.guest_debug & KVM_GUESTDBG_SINGLESTEP)) {
> >  			svm->vmcb->save.rflags &=
> >  				~(X86_EFLAGS_TF | X86_EFLAGS_RF);
> > +			if (svm->nmi_singlestep_tf) {
> > +				svm->vmcb->save.rflags |= X86_EFLAGS_TF;
> > +				kvm_queue_exception(&svm->vcpu, DB_VECTOR);
> > +			}
> > +		}
> >  		update_db_intercept(&svm->vcpu);
> >  	}
> >  
> > @@ -2586,6 +2592,7 @@ static void enable_nmi_window(struct kvm_vcpu *vcpu)
> >  	   possible problem (IRET or exception injection or interrupt
> >  	   shadow) */
> >  	svm->nmi_singlestep = true;
> > +	svm->nmi_singlestep_tf = (svm->vmcb->save.rflags | X86_EFLAGS_TF);
> >  	svm->vmcb->save.rflags |= (X86_EFLAGS_TF | X86_EFLAGS_RF);
> >  	update_db_intercept(vcpu);
> >  }
> 
> That's closer. However, I've a version here that restores TF&RF only if
> you did not execute an IRET but stepped over the shadow (which is still
> not correct either, e.g. when stepping popf). I will break up my patch
> into parts that fix the issues separately so that we can decide what to
> merge.
> 
I am not sure what do you mean here. Why should we restore RF? It is
cleared after each instruction execution and popf is not special in this
regards and SDM explicitly says so.

--
			Gleb.