[PATCH 0/3] Fix task switches into/out of VM86

[PATCH 0/3] Fix task switches into/out of VM86

[Kevin Wolf]

Kevin Wolf (3):
  KVM: x86 emulator: Fix task switch privilege checks
  KVM: x86 emulator: VM86 segments must have DPL 3
  KVM: x86 emulator: Allow PM/VM86 switch during task switch

 arch/x86/include/asm/kvm_emulate.h |    3 +-
 arch/x86/include/asm/kvm_host.h    |    4 +-
 arch/x86/kvm/emulate.c             |   70 ++++++++++++++++++++++++++++++++----
 arch/x86/kvm/svm.c                 |    3 +-
 arch/x86/kvm/vmx.c                 |    5 ++-
 arch/x86/kvm/x86.c                 |   12 +++++--
 6 files changed, 81 insertions(+), 16 deletions(-)

-- 
1.7.6.5

[PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Currently, all task switches check privileges against the DPL of the
TSS. This is only correct for jmp/call to a TSS. If a task gate is used,
the DPL of this take gate is used for the check instead. Exceptions,
external interrupts and iret shouldn't perform any check.

This patch fixes the problem for VMX. For SVM, the logic used to
determine the source of the task switch is buggy, so we can't pass
useful information to the emulator there and just disable the check in
all cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 arch/x86/include/asm/kvm_emulate.h |    2 +-
 arch/x86/include/asm/kvm_host.h    |    4 +-
 arch/x86/kvm/emulate.c             |   51 +++++++++++++++++++++++++++++++-----
 arch/x86/kvm/svm.c                 |    3 +-
 arch/x86/kvm/vmx.c                 |    5 ++-
 arch/x86/kvm/x86.c                 |    6 ++--
 6 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index ab4092e..c8a9cf3 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -372,7 +372,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);
 #define EMULATION_INTERCEPTED 2
 int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
 int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
-			 u16 tss_selector, int reason,
+			 u16 tss_selector, int idt_index, int reason,
 			 bool has_error_code, u32 error_code);
 int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
 #endif /* _ASM_X86_KVM_X86_EMULATE_H */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 52d6640..0533fc4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -741,8 +741,8 @@ int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
 void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
 int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector, int seg);
 
-int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
-		    bool has_error_code, u32 error_code);
+int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
+		    int reason, bool has_error_code, u32 error_code);
 
 int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
 int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 05a562b..1b98a2c 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1151,6 +1151,22 @@ static int pio_in_emulated(struct x86_emulate_ctxt *ctxt,
 	return 1;
 }
 
+static int read_interrupt_descriptor(struct x86_emulate_ctxt *ctxt,
+				     u16 index, struct kvm_desc_struct *desc)
+{
+	struct kvm_desc_ptr dt;
+	ulong addr;
+
+	ctxt->ops->get_idt(ctxt, &dt);
+
+	if (dt.size < index * 8 + 7)
+		return emulate_gp(ctxt, index << 3 | 0x2);
+
+	addr = dt.address + index * 8;
+	return ctxt->ops->read_std(ctxt, addr, desc, sizeof *desc,
+				   &ctxt->exception);
+}
+
 static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
 				     u16 selector, struct desc_ptr *dt)
 {
@@ -2350,7 +2366,7 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
 }
 
 static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
-				   u16 tss_selector, int reason,
+				   u16 tss_selector, int idt_index, int reason,
 				   bool has_error_code, u32 error_code)
 {
 	struct x86_emulate_ops *ops = ctxt->ops;
@@ -2360,6 +2376,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 	ulong old_tss_base =
 		ops->get_cached_segment_base(ctxt, VCPU_SREG_TR);
 	u32 desc_limit;
+	int dpl;
 
 	/* FIXME: old_tss_base == ~0 ? */
 
@@ -2372,12 +2389,32 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 
 	/* FIXME: check that next_tss_desc is tss */
 
-	if (reason != TASK_SWITCH_IRET) {
-		if ((tss_selector & 3) > next_tss_desc.dpl ||
-		    ops->cpl(ctxt) > next_tss_desc.dpl)
-			return emulate_gp(ctxt, 0);
+	/*
+	 * Check privileges. The three cases are task switch caused by...
+	 *
+	 * 1. Software interrupt: Check against DPL of the task gate
+	 * 2. Exception/IRQ/iret: No check is performed
+	 * 3. jmp/call: Check agains DPL of the TSS
+	 */
+	dpl = -1;
+	if (reason == TASK_SWITCH_GATE) {
+		if (idt_index != -1) {
+			struct kvm_desc_struct task_gate_desc;
+
+			ret = read_interrupt_descriptor(ctxt, idt_index,
+							&task_gate_desc);
+			if (ret != X86EMUL_CONTINUE)
+				return ret;
+
+			dpl = task_gate_desc.dpl;
+		}
+	} else if (reason != TASK_SWITCH_IRET) {
+		dpl = next_tss_desc.dpl;
 	}
 
+	if (dpl != -1 && ((tss_selector & 3) > dpl || ops->cpl(ctxt) > dpl))
+		return emulate_gp(ctxt, 0);
+
 	desc_limit = desc_limit_scaled(&next_tss_desc);
 	if (!next_tss_desc.p ||
 	    ((desc_limit < 0x67 && (next_tss_desc.type & 8)) ||
@@ -2430,7 +2467,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 }
 
 int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
-			 u16 tss_selector, int reason,
+			 u16 tss_selector, int idt_index, int reason,
 			 bool has_error_code, u32 error_code)
 {
 	int rc;
@@ -2438,7 +2475,7 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
 	ctxt->_eip = ctxt->eip;
 	ctxt->dst.type = OP_NONE;
 
-	rc = emulator_do_task_switch(ctxt, tss_selector, reason,
+	rc = emulator_do_task_switch(ctxt, tss_selector, idt_index, reason,
 				     has_error_code, error_code);
 
 	if (rc == X86EMUL_CONTINUE)
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 5fa553b..b2223dc 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2730,7 +2730,8 @@ static int task_switch_interception(struct vcpu_svm *svm)
 	     (int_vec == OF_VECTOR || int_vec == BP_VECTOR)))
 		skip_emulated_instruction(&svm->vcpu);
 
-	if (kvm_task_switch(&svm->vcpu, tss_selector, reason,
+	/* TODO Pass IDT vector for software interrupts */
+	if (kvm_task_switch(&svm->vcpu, tss_selector, -1, reason,
 				has_error_code, error_code) == EMULATE_FAIL) {
 		svm->vcpu.run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		svm->vcpu.run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 906a7e8..23fb328 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4712,8 +4712,9 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
 		       type != INTR_TYPE_NMI_INTR))
 		skip_emulated_instruction(vcpu);
 
-	if (kvm_task_switch(vcpu, tss_selector, reason,
-				has_error_code, error_code) == EMULATE_FAIL) {
+	if (kvm_task_switch(vcpu, tss_selector,
+			    type == INTR_TYPE_SOFT_INTR ? idt_v : -1, reason,
+			    has_error_code, error_code) == EMULATE_FAIL) {
 		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
 		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
 		vcpu->run->internal.ndata = 0;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1171def..dc3e945 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5541,15 +5541,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
-int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
-		    bool has_error_code, u32 error_code)
+int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
+		    int reason, bool has_error_code, u32 error_code)
 {
 	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
 	int ret;
 
 	init_emulate_ctxt(vcpu);
 
-	ret = emulator_task_switch(ctxt, tss_selector, reason,
+	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
 				   has_error_code, error_code);
 
 	if (ret)
-- 
1.7.6.5

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
> Currently, all task switches check privileges against the DPL of the
> TSS. This is only correct for jmp/call to a TSS. If a task gate is used,
> the DPL of this take gate is used for the check instead. Exceptions,
> external interrupts and iret shouldn't perform any check.
> 
> This patch fixes the problem for VMX. For SVM, the logic used to
> determine the source of the task switch is buggy, so we can't pass
> useful information to the emulator there and just disable the check in
> all cases.
> 
Is this accurate description? Since on SVM you never get
TASK_SWITCH_GATE reason the check is always done using TSS dpl, not
disabled in all cases, correct?

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  arch/x86/include/asm/kvm_emulate.h |    2 +-
>  arch/x86/include/asm/kvm_host.h    |    4 +-
>  arch/x86/kvm/emulate.c             |   51 +++++++++++++++++++++++++++++++-----
>  arch/x86/kvm/svm.c                 |    3 +-
>  arch/x86/kvm/vmx.c                 |    5 ++-
>  arch/x86/kvm/x86.c                 |    6 ++--
>  6 files changed, 55 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index ab4092e..c8a9cf3 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -372,7 +372,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);
>  #define EMULATION_INTERCEPTED 2
>  int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
>  int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
> -			 u16 tss_selector, int reason,
> +			 u16 tss_selector, int idt_index, int reason,
>  			 bool has_error_code, u32 error_code);
>  int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
>  #endif /* _ASM_X86_KVM_X86_EMULATE_H */
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 52d6640..0533fc4 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -741,8 +741,8 @@ int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
>  void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
>  int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector, int seg);
>  
> -int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
> -		    bool has_error_code, u32 error_code);
> +int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
> +		    int reason, bool has_error_code, u32 error_code);
>  
>  int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
>  int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 05a562b..1b98a2c 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1151,6 +1151,22 @@ static int pio_in_emulated(struct x86_emulate_ctxt *ctxt,
>  	return 1;
>  }
>  
> +static int read_interrupt_descriptor(struct x86_emulate_ctxt *ctxt,
> +				     u16 index, struct kvm_desc_struct *desc)
> +{
> +	struct kvm_desc_ptr dt;
> +	ulong addr;
> +
> +	ctxt->ops->get_idt(ctxt, &dt);
> +
> +	if (dt.size < index * 8 + 7)
> +		return emulate_gp(ctxt, index << 3 | 0x2);
> +
> +	addr = dt.address + index * 8;
> +	return ctxt->ops->read_std(ctxt, addr, desc, sizeof *desc,
> +				   &ctxt->exception);
> +}
> +
>  static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
>  				     u16 selector, struct desc_ptr *dt)
>  {
> @@ -2350,7 +2366,7 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
>  }
>  
>  static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
> -				   u16 tss_selector, int reason,
> +				   u16 tss_selector, int idt_index, int reason,
>  				   bool has_error_code, u32 error_code)
>  {
>  	struct x86_emulate_ops *ops = ctxt->ops;
> @@ -2360,6 +2376,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  	ulong old_tss_base =
>  		ops->get_cached_segment_base(ctxt, VCPU_SREG_TR);
>  	u32 desc_limit;
> +	int dpl;
>  
>  	/* FIXME: old_tss_base == ~0 ? */
>  
> @@ -2372,12 +2389,32 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  
>  	/* FIXME: check that next_tss_desc is tss */
>  
> -	if (reason != TASK_SWITCH_IRET) {
> -		if ((tss_selector & 3) > next_tss_desc.dpl ||
> -		    ops->cpl(ctxt) > next_tss_desc.dpl)
> -			return emulate_gp(ctxt, 0);
> +	/*
> +	 * Check privileges. The three cases are task switch caused by...
> +	 *
> +	 * 1. Software interrupt: Check against DPL of the task gate
> +	 * 2. Exception/IRQ/iret: No check is performed
> +	 * 3. jmp/call: Check agains DPL of the TSS
> +	 */
> +	dpl = -1;
> +	if (reason == TASK_SWITCH_GATE) {
> +		if (idt_index != -1) {
> +			struct kvm_desc_struct task_gate_desc;
> +
> +			ret = read_interrupt_descriptor(ctxt, idt_index,
> +							&task_gate_desc);
> +			if (ret != X86EMUL_CONTINUE)
> +				return ret;
> +
> +			dpl = task_gate_desc.dpl;
> +		}
> +	} else if (reason != TASK_SWITCH_IRET) {
> +		dpl = next_tss_desc.dpl;
>  	}
No need parentheses around one statement.

>  
> +	if (dpl != -1 && ((tss_selector & 3) > dpl || ops->cpl(ctxt) > dpl))
> +		return emulate_gp(ctxt, 0);
> +
>  	desc_limit = desc_limit_scaled(&next_tss_desc);
>  	if (!next_tss_desc.p ||
>  	    ((desc_limit < 0x67 && (next_tss_desc.type & 8)) ||
> @@ -2430,7 +2467,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>  }
>  
>  int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
> -			 u16 tss_selector, int reason,
> +			 u16 tss_selector, int idt_index, int reason,
>  			 bool has_error_code, u32 error_code)
>  {
>  	int rc;
> @@ -2438,7 +2475,7 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
>  	ctxt->_eip = ctxt->eip;
>  	ctxt->dst.type = OP_NONE;
>  
> -	rc = emulator_do_task_switch(ctxt, tss_selector, reason,
> +	rc = emulator_do_task_switch(ctxt, tss_selector, idt_index, reason,
>  				     has_error_code, error_code);
>  
>  	if (rc == X86EMUL_CONTINUE)
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5fa553b..b2223dc 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2730,7 +2730,8 @@ static int task_switch_interception(struct vcpu_svm *svm)
>  	     (int_vec == OF_VECTOR || int_vec == BP_VECTOR)))
>  		skip_emulated_instruction(&svm->vcpu);
>  
> -	if (kvm_task_switch(&svm->vcpu, tss_selector, reason,
> +	/* TODO Pass IDT vector for software interrupts */
> +	if (kvm_task_switch(&svm->vcpu, tss_selector, -1, reason,
>  				has_error_code, error_code) == EMULATE_FAIL) {
>  		svm->vcpu.run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  		svm->vcpu.run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 906a7e8..23fb328 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4712,8 +4712,9 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
>  		       type != INTR_TYPE_NMI_INTR))
>  		skip_emulated_instruction(vcpu);
>  
> -	if (kvm_task_switch(vcpu, tss_selector, reason,
> -				has_error_code, error_code) == EMULATE_FAIL) {
> +	if (kvm_task_switch(vcpu, tss_selector,
> +			    type == INTR_TYPE_SOFT_INTR ? idt_v : -1, reason,
> +			    has_error_code, error_code) == EMULATE_FAIL) {
>  		vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>  		vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
>  		vcpu->run->internal.ndata = 0;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1171def..dc3e945 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5541,15 +5541,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> -int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
> -		    bool has_error_code, u32 error_code)
> +int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
> +		    int reason, bool has_error_code, u32 error_code)
>  {
>  	struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
>  	int ret;
>  
>  	init_emulate_ctxt(vcpu);
>  
> -	ret = emulator_task_switch(ctxt, tss_selector, reason,
> +	ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
>  				   has_error_code, error_code);
>  
>  	if (ret)
> -- 
> 1.7.6.5

--
			Gleb.

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 24.01.2012 10:52, schrieb Gleb Natapov:
> On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
>> Currently, all task switches check privileges against the DPL of the
>> TSS. This is only correct for jmp/call to a TSS. If a task gate is used,
>> the DPL of this take gate is used for the check instead. Exceptions,
>> external interrupts and iret shouldn't perform any check.
>>
>> This patch fixes the problem for VMX. For SVM, the logic used to
>> determine the source of the task switch is buggy, so we can't pass
>> useful information to the emulator there and just disable the check in
>> all cases.
>>
> Is this accurate description? Since on SVM you never get
> TASK_SWITCH_GATE reason the check is always done using TSS dpl, not
> disabled in all cases, correct?

Hm, right, they turn out as TASK_SWITCH_CALL, so we check against the
TSS. I'll fix the commit message if I need to do a v2.

>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_emulate.h |    2 +-
>>  arch/x86/include/asm/kvm_host.h    |    4 +-
>>  arch/x86/kvm/emulate.c             |   51 +++++++++++++++++++++++++++++++-----
>>  arch/x86/kvm/svm.c                 |    3 +-
>>  arch/x86/kvm/vmx.c                 |    5 ++-
>>  arch/x86/kvm/x86.c                 |    6 ++--
>>  6 files changed, 55 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
>> index ab4092e..c8a9cf3 100644
>> --- a/arch/x86/include/asm/kvm_emulate.h
>> +++ b/arch/x86/include/asm/kvm_emulate.h
>> @@ -372,7 +372,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);
>>  #define EMULATION_INTERCEPTED 2
>>  int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
>>  int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
>> -			 u16 tss_selector, int reason,
>> +			 u16 tss_selector, int idt_index, int reason,
>>  			 bool has_error_code, u32 error_code);
>>  int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
>>  #endif /* _ASM_X86_KVM_X86_EMULATE_H */
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 52d6640..0533fc4 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -741,8 +741,8 @@ int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
>>  void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
>>  int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector, int seg);
>>  
>> -int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
>> -		    bool has_error_code, u32 error_code);
>> +int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>> +		    int reason, bool has_error_code, u32 error_code);
>>  
>>  int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
>>  int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 05a562b..1b98a2c 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -1151,6 +1151,22 @@ static int pio_in_emulated(struct x86_emulate_ctxt *ctxt,
>>  	return 1;
>>  }
>>  
>> +static int read_interrupt_descriptor(struct x86_emulate_ctxt *ctxt,
>> +				     u16 index, struct kvm_desc_struct *desc)
>> +{
>> +	struct kvm_desc_ptr dt;
>> +	ulong addr;
>> +
>> +	ctxt->ops->get_idt(ctxt, &dt);
>> +
>> +	if (dt.size < index * 8 + 7)
>> +		return emulate_gp(ctxt, index << 3 | 0x2);
>> +
>> +	addr = dt.address + index * 8;
>> +	return ctxt->ops->read_std(ctxt, addr, desc, sizeof *desc,
>> +				   &ctxt->exception);
>> +}
>> +
>>  static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
>>  				     u16 selector, struct desc_ptr *dt)
>>  {
>> @@ -2350,7 +2366,7 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
>>  }
>>  
>>  static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>> -				   u16 tss_selector, int reason,
>> +				   u16 tss_selector, int idt_index, int reason,
>>  				   bool has_error_code, u32 error_code)
>>  {
>>  	struct x86_emulate_ops *ops = ctxt->ops;
>> @@ -2360,6 +2376,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>>  	ulong old_tss_base =
>>  		ops->get_cached_segment_base(ctxt, VCPU_SREG_TR);
>>  	u32 desc_limit;
>> +	int dpl;
>>  
>>  	/* FIXME: old_tss_base == ~0 ? */
>>  
>> @@ -2372,12 +2389,32 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>>  
>>  	/* FIXME: check that next_tss_desc is tss */
>>  
>> -	if (reason != TASK_SWITCH_IRET) {
>> -		if ((tss_selector & 3) > next_tss_desc.dpl ||
>> -		    ops->cpl(ctxt) > next_tss_desc.dpl)
>> -			return emulate_gp(ctxt, 0);
>> +	/*
>> +	 * Check privileges. The three cases are task switch caused by...
>> +	 *
>> +	 * 1. Software interrupt: Check against DPL of the task gate
>> +	 * 2. Exception/IRQ/iret: No check is performed
>> +	 * 3. jmp/call: Check agains DPL of the TSS
>> +	 */
>> +	dpl = -1;
>> +	if (reason == TASK_SWITCH_GATE) {
>> +		if (idt_index != -1) {
>> +			struct kvm_desc_struct task_gate_desc;
>> +
>> +			ret = read_interrupt_descriptor(ctxt, idt_index,
>> +							&task_gate_desc);
>> +			if (ret != X86EMUL_CONTINUE)
>> +				return ret;
>> +
>> +			dpl = task_gate_desc.dpl;
>> +		}
>> +	} else if (reason != TASK_SWITCH_IRET) {
>> +		dpl = next_tss_desc.dpl;
>>  	}
> No need parentheses around one statement.

Documentation/CodingStyle says:

"This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:"

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
> >> +	} else if (reason != TASK_SWITCH_IRET) {
> >> +		dpl = next_tss_desc.dpl;
> >>  	}
> > No need parentheses around one statement.
> 
> Documentation/CodingStyle says:
> 
> "This does not apply if only one branch of a conditional statement is a
> single statement; in the latter case use braces in both branches:"
> 
Then you need to put parentheses around "if (reason != TASK_SWITCH_IRET)"
if you want to follow the letter of the CodingStyle :)
But I do not see this coding stile part widely used in core kernel code:
$ git grep "} else$" kernel | wc -l
122

Can't think of re to check when the rule is followed :(

--
			Gleb.

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 24.01.2012 11:17, schrieb Gleb Natapov:
> On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
>>>> +	} else if (reason != TASK_SWITCH_IRET) {
>>>> +		dpl = next_tss_desc.dpl;
>>>>  	}
>>> No need parentheses around one statement.
>>
>> Documentation/CodingStyle says:
>>
>> "This does not apply if only one branch of a conditional statement is a
>> single statement; in the latter case use braces in both branches:"
>>
> Then you need to put parentheses around "if (reason != TASK_SWITCH_IRET)"
> if you want to follow the letter of the CodingStyle :)

Not sure what you mean. If it is 'else { if (...) { ..." then no, the
document isn't crazy like that.

> But I do not see this coding stile part widely used in core kernel code:
> $ git grep "} else$" kernel | wc -l
> 122
> 
> Can't think of re to check when the rule is followed :(

Seem to be at least 77 occurences (git grep -A 2 "} else {" into a file
as git grep doesn't seem to do multi-line expressions and then on that
file "} else {\n.*\n.*}$")

But anyway, I don't really want to discuss the right coding style here
and I'll apply whatever is considered right. Though if you think that
checkpatch.pl and Documentation/CodingStyle are both wrong, please get
them fixed. People might take them serious.

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Tue, Jan 24, 2012 at 11:38:24AM +0100, Kevin Wolf wrote:
> Am 24.01.2012 11:17, schrieb Gleb Natapov:
> > On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
> >>>> +	} else if (reason != TASK_SWITCH_IRET) {
> >>>> +		dpl = next_tss_desc.dpl;
> >>>>  	}
> >>> No need parentheses around one statement.
> >>
> >> Documentation/CodingStyle says:
> >>
> >> "This does not apply if only one branch of a conditional statement is a
> >> single statement; in the latter case use braces in both branches:"
> >>
> > Then you need to put parentheses around "if (reason != TASK_SWITCH_IRET)"
> > if you want to follow the letter of the CodingStyle :)
> 
> Not sure what you mean. If it is 'else { if (...) { ..." then no, the
> document isn't crazy like that.
> 
The document says:

if (condition) {
        do_this();
        do_that();
} else {
        otherwise();
}

So I do not see how you can interpret it otherwise.

> > But I do not see this coding stile part widely used in core kernel code:
> > $ git grep "} else$" kernel | wc -l
> > 122
> > 
> > Can't think of re to check when the rule is followed :(
> 
> Seem to be at least 77 occurences (git grep -A 2 "} else {" into a file
> as git grep doesn't seem to do multi-line expressions and then on that
> file "} else {\n.*\n.*}$")
> 
> But anyway, I don't really want to discuss the right coding style here
> and I'll apply whatever is considered right. Though if you think that
> checkpatch.pl and Documentation/CodingStyle are both wrong, please get
> them fixed. People might take them serious.
> 
The code looked strange for kernel code. If checkpatch.pl complains about
missing parentheses then they should of course stay. Our interpretation
of CodingStyle is different.

--
			Gleb.

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 24.01.2012 11:52, schrieb Gleb Natapov:
> On Tue, Jan 24, 2012 at 11:38:24AM +0100, Kevin Wolf wrote:
>> Am 24.01.2012 11:17, schrieb Gleb Natapov:
>>> On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
>>>>>> +	} else if (reason != TASK_SWITCH_IRET) {
>>>>>> +		dpl = next_tss_desc.dpl;
>>>>>>  	}
>>>>> No need parentheses around one statement.
>>>>
>>>> Documentation/CodingStyle says:
>>>>
>>>> "This does not apply if only one branch of a conditional statement is a
>>>> single statement; in the latter case use braces in both branches:"
>>>>
>>> Then you need to put parentheses around "if (reason != TASK_SWITCH_IRET)"
>>> if you want to follow the letter of the CodingStyle :)
>>
>> Not sure what you mean. If it is 'else { if (...) { ..." then no, the
>> document isn't crazy like that.
>>
> The document says:
> 
> if (condition) {
>         do_this();
>         do_that();
> } else {
>         otherwise();
> }
> 
> So I do not see how you can interpret it otherwise.

Well, I just read more than only one paragraph. It further says:

"Note that the closing brace is empty on a line of its own, _except_ in
the cases where it is followed by a continuation of the same statement,
ie a "while" in a do-statement or an "else" in an if-statement, like
this:

        if (x == y) {
                ..
        } else if (x > y) {
                ...
        } else {
                ....
        }
"

> 
>>> But I do not see this coding stile part widely used in core kernel code:
>>> $ git grep "} else$" kernel | wc -l
>>> 122
>>>
>>> Can't think of re to check when the rule is followed :(
>>
>> Seem to be at least 77 occurences (git grep -A 2 "} else {" into a file
>> as git grep doesn't seem to do multi-line expressions and then on that
>> file "} else {\n.*\n.*}$")
>>
>> But anyway, I don't really want to discuss the right coding style here
>> and I'll apply whatever is considered right. Though if you think that
>> checkpatch.pl and Documentation/CodingStyle are both wrong, please get
>> them fixed. People might take them serious.
>>
> The code looked strange for kernel code. If checkpatch.pl complains about
> missing parentheses then they should of course stay. Our interpretation
> of CodingStyle is different.

checkpatch.pl accepts both ways, and as the counts above show they are
both used in core kernel code. If Avi or Marcelo want to have it changed
anyway, I'll resend.

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Tue, Jan 24, 2012 at 12:23:07PM +0100, Kevin Wolf wrote:
> Am 24.01.2012 11:52, schrieb Gleb Natapov:
> > On Tue, Jan 24, 2012 at 11:38:24AM +0100, Kevin Wolf wrote:
> >> Am 24.01.2012 11:17, schrieb Gleb Natapov:
> >>> On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
> >>>>>> +	} else if (reason != TASK_SWITCH_IRET) {
> >>>>>> +		dpl = next_tss_desc.dpl;
> >>>>>>  	}
> >>>>> No need parentheses around one statement.
> >>>>
> >>>> Documentation/CodingStyle says:
> >>>>
> >>>> "This does not apply if only one branch of a conditional statement is a
> >>>> single statement; in the latter case use braces in both branches:"
> >>>>
> >>> Then you need to put parentheses around "if (reason != TASK_SWITCH_IRET)"
> >>> if you want to follow the letter of the CodingStyle :)
> >>
> >> Not sure what you mean. If it is 'else { if (...) { ..." then no, the
> >> document isn't crazy like that.
> >>
> > The document says:
> > 
> > if (condition) {
> >         do_this();
> >         do_that();
> > } else {
> >         otherwise();
> > }
> > 
> > So I do not see how you can interpret it otherwise.
> 
> Well, I just read more than only one paragraph. It further says:
> 
> "Note that the closing brace is empty on a line of its own, _except_ in
> the cases where it is followed by a continuation of the same statement,
> ie a "while" in a do-statement or an "else" in an if-statement, like
> this:
> 
>         if (x == y) {
>                 ..
>         } else if (x > y) {
>                 ...
>         } else {
>                 ....
>         }
> "
> 
I saw that, but here it talks about something different: if closing
brace should be the only thing on a line or not. It does not specify how
much statements ... represents. Example is chosen to show the point
paragraph before it tries to make and for that you need braces.

> > 
> >>> But I do not see this coding stile part widely used in core kernel code:
> >>> $ git grep "} else$" kernel | wc -l
> >>> 122
> >>>
> >>> Can't think of re to check when the rule is followed :(
> >>
> >> Seem to be at least 77 occurences (git grep -A 2 "} else {" into a file
> >> as git grep doesn't seem to do multi-line expressions and then on that
> >> file "} else {\n.*\n.*}$")
> >>
> >> But anyway, I don't really want to discuss the right coding style here
> >> and I'll apply whatever is considered right. Though if you think that
> >> checkpatch.pl and Documentation/CodingStyle are both wrong, please get
> >> them fixed. People might take them serious.
> >>
> > The code looked strange for kernel code. If checkpatch.pl complains about
> > missing parentheses then they should of course stay. Our interpretation
> > of CodingStyle is different.
> 
> checkpatch.pl accepts both ways, and as the counts above show they are
> both used in core kernel code. If Avi or Marcelo want to have it changed
> anyway, I'll resend.
> 

--
			Gleb.

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Joerg Roedel]

On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
> This patch fixes the problem for VMX. For SVM, the logic used to
> determine the source of the task switch is buggy, so we can't pass
> useful information to the emulator there and just disable the check in
> all cases.

Actually, SVM isn't buggy :) For SVM you do not need to do any
priviledge checks in software because the hardware already takes care of
that.
In other words, KVM only gets a task-switch intercept if the priviledges
are all checked and correct.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 24.01.2012 15:03, schrieb Joerg Roedel:
> On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
>> This patch fixes the problem for VMX. For SVM, the logic used to
>> determine the source of the task switch is buggy, so we can't pass
>> useful information to the emulator there and just disable the check in
>> all cases.
> 
> Actually, SVM isn't buggy :) For SVM you do not need to do any
> priviledge checks in software because the hardware already takes care of
> that.
> In other words, KVM only gets a task-switch intercept if the priviledges
> are all checked and correct.

Okay, that's good to hear. The current code is still buggy because as
Gleb noted it checks against the TSS DPL. We need to disable that check
for SVM then. Also all checks for TASK_SWITCH_GATE indicate that
something is wrong because it will never happen.

Are you going to rewrite task_switch_interception() on top of this series?

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Tue, Jan 24, 2012 at 03:15:13PM +0100, Kevin Wolf wrote:
> Am 24.01.2012 15:03, schrieb Joerg Roedel:
> > On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
> >> This patch fixes the problem for VMX. For SVM, the logic used to
> >> determine the source of the task switch is buggy, so we can't pass
> >> useful information to the emulator there and just disable the check in
> >> all cases.
> > 
> > Actually, SVM isn't buggy :) For SVM you do not need to do any
> > priviledge checks in software because the hardware already takes care of
> > that.
> > In other words, KVM only gets a task-switch intercept if the priviledges
> > are all checked and correct.
> 
> Okay, that's good to hear. The current code is still buggy because as
> Gleb noted it checks against the TSS DPL. We need to disable that check
> for SVM then. Also all checks for TASK_SWITCH_GATE indicate that
> something is wrong because it will never happen.
> 
Not necessary. Currently all checks for TASK_SWITCH_GATE also check for
TASK_SWITCH_CALL, so I think you can fix SVM case in your patch by
passing TASK_SWITCH_GATE instead of TASK_SWITCH_CALL to
kvm_task_switch().

> Are you going to rewrite task_switch_interception() on top of this series?
> 
> Kevin

--
			Gleb.

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 24.01.2012 15:16, schrieb Gleb Natapov:
> On Tue, Jan 24, 2012 at 03:15:13PM +0100, Kevin Wolf wrote:
>> Am 24.01.2012 15:03, schrieb Joerg Roedel:
>>> On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
>>>> This patch fixes the problem for VMX. For SVM, the logic used to
>>>> determine the source of the task switch is buggy, so we can't pass
>>>> useful information to the emulator there and just disable the check in
>>>> all cases.
>>>
>>> Actually, SVM isn't buggy :) For SVM you do not need to do any
>>> priviledge checks in software because the hardware already takes care of
>>> that.
>>> In other words, KVM only gets a task-switch intercept if the priviledges
>>> are all checked and correct.
>>
>> Okay, that's good to hear. The current code is still buggy because as
>> Gleb noted it checks against the TSS DPL. We need to disable that check
>> for SVM then. Also all checks for TASK_SWITCH_GATE indicate that
>> something is wrong because it will never happen.
>>
> Not necessary. Currently all checks for TASK_SWITCH_GATE also check for
> TASK_SWITCH_CALL, so I think you can fix SVM case in your patch by
> passing TASK_SWITCH_GATE instead of TASK_SWITCH_CALL to
> kvm_task_switch().

Yes, the emulator itself would be fixed by passing TASK_SWITCH_GATE and
idt_index = -1 (although it looks a bit brittle).

However, task_switch_interception() itself does some more based on the
value of reason, for example it decides whether or not to call
skip_emulated_instruction().

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote:
> Am 24.01.2012 15:16, schrieb Gleb Natapov:
> > On Tue, Jan 24, 2012 at 03:15:13PM +0100, Kevin Wolf wrote:
> >> Am 24.01.2012 15:03, schrieb Joerg Roedel:
> >>> On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
> >>>> This patch fixes the problem for VMX. For SVM, the logic used to
> >>>> determine the source of the task switch is buggy, so we can't pass
> >>>> useful information to the emulator there and just disable the check in
> >>>> all cases.
> >>>
> >>> Actually, SVM isn't buggy :) For SVM you do not need to do any
> >>> priviledge checks in software because the hardware already takes care of
> >>> that.
> >>> In other words, KVM only gets a task-switch intercept if the priviledges
> >>> are all checked and correct.
> >>
> >> Okay, that's good to hear. The current code is still buggy because as
> >> Gleb noted it checks against the TSS DPL. We need to disable that check
> >> for SVM then. Also all checks for TASK_SWITCH_GATE indicate that
> >> something is wrong because it will never happen.
> >>
> > Not necessary. Currently all checks for TASK_SWITCH_GATE also check for
> > TASK_SWITCH_CALL, so I think you can fix SVM case in your patch by
> > passing TASK_SWITCH_GATE instead of TASK_SWITCH_CALL to
> > kvm_task_switch().
> 
> Yes, the emulator itself would be fixed by passing TASK_SWITCH_GATE and
> idt_index = -1 (although it looks a bit brittle).
> 
I think it is OK. Emulator should emulate the spec and if workaround is
needed it should be in platform code. By passing TASK_SWITCH_GATE and
idt_index = -1 we do exactly that.

> However, task_switch_interception() itself does some more based on the
> value of reason, for example it decides whether or not to call
> skip_emulated_instruction().
> 
Joerg need to help us here. If intercept of task switch happens before
rip is advanced past instruction that cause it we have to know somehow
that task switch was caused by instruction. It is not enough that HW
checks permission, we still lack essential info.

--
			Gleb.

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Joerg Roedel]

On Tue, Jan 24, 2012 at 06:23:50PM +0200, Gleb Natapov wrote:
> On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote:

> > However, task_switch_interception() itself does some more based on the
> > value of reason, for example it decides whether or not to call
> > skip_emulated_instruction().
> > 
> Joerg need to help us here. If intercept of task switch happens before
> rip is advanced past instruction that cause it we have to know somehow
> that task switch was caused by instruction. It is not enough that HW
> checks permission, we still lack essential info.

Hmm, the RIP in the VMCB points to the instruction causing the task
switch. This is also true for lcall and ljmp. But in my experiments I
have seen exit_int_info.valid = 1 for task-switches that went through
the IDT. But I havn't tested the VM86 case, though.

Kevin, can you please re-verify that exit_int_info.valid is always 0 in
your experiment? On what hardware have you tested this?

Thanks,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Wed, Jan 25, 2012 at 05:00:58PM +0100, Joerg Roedel wrote:
> On Tue, Jan 24, 2012 at 06:23:50PM +0200, Gleb Natapov wrote:
> > On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote:
> 
> > > However, task_switch_interception() itself does some more based on the
> > > value of reason, for example it decides whether or not to call
> > > skip_emulated_instruction().
> > > 
> > Joerg need to help us here. If intercept of task switch happens before
> > rip is advanced past instruction that cause it we have to know somehow
> > that task switch was caused by instruction. It is not enough that HW
> > checks permission, we still lack essential info.
> 
> Hmm, the RIP in the VMCB points to the instruction causing the task
> switch. This is also true for lcall and ljmp. But in my experiments I
> have seen exit_int_info.valid = 1 for task-switches that went through
> the IDT. But I havn't tested the VM86 case, though.
> 
I can confirm that I get exit_int_info.valid = 1 for all scenarios when
task switch is caused by idt event. Just checked it here.

> Kevin, can you please re-verify that exit_int_info.valid is always 0 in
> your experiment? On what hardware have you tested this?
> 
> Thanks,
> 
> 	Joerg
> 
> -- 
> AMD Operating System Research Center
> 
> Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
> General Managers: Alberto Bozzo
> Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

--
			Gleb.

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 25.01.2012 17:00, schrieb Joerg Roedel:
> On Tue, Jan 24, 2012 at 06:23:50PM +0200, Gleb Natapov wrote:
>> On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote:
> 
>>> However, task_switch_interception() itself does some more based on the
>>> value of reason, for example it decides whether or not to call
>>> skip_emulated_instruction().
>>>
>> Joerg need to help us here. If intercept of task switch happens before
>> rip is advanced past instruction that cause it we have to know somehow
>> that task switch was caused by instruction. It is not enough that HW
>> checks permission, we still lack essential info.
> 
> Hmm, the RIP in the VMCB points to the instruction causing the task
> switch. This is also true for lcall and ljmp. But in my experiments I
> have seen exit_int_info.valid = 1 for task-switches that went through
> the IDT. But I havn't tested the VM86 case, though.
> 
> Kevin, can you please re-verify that exit_int_info.valid is always 0 in
> your experiment? On what hardware have you tested this?

I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus
the tree patches of this series plus a printk to output exit_int_info in
task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got
two failures and my VM86 unit test which hung when trying to return from
VM86. I also ran the kernel that made me aware of the issue initially.
All debug messages show exit_int_info = 0.

This is the /proc/cpuinfo snippet for the first core:

processor       : 0
vendor_id       : AuthenticAMD
cpu family      : 15
model           : 107
model name      : AMD Athlon(tm) 64 X2 Dual Core Processor 5200+
stepping        : 2
cpu MHz         : 1800.000
cache size      : 512 KB
physical id     : 0
siblings        : 2
core id         : 0
cpu cores       : 2
apicid          : 0
initial apicid  : 0
fpu             : yes
fpu_exception   : yes
cpuid level     : 1
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge
mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext
fxsr_opt rdtscp lm 3dnowext 3dnow rep_good nopl extd_apicid pni cx16
lahf_lm cmp_legacy svm extapic cr8_legacy 3dnowprefetch lbrv
bogomips        : 3592.64
TLB size        : 1024 4K pages
clflush size    : 64
cache_alignment : 64
address sizes   : 40 bits physical, 48 bits virtual
power management: ts fid vid ttp tm stc 100mhzsteps

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Joerg Roedel]

On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote:
> Am 25.01.2012 17:00, schrieb Joerg Roedel:

> I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus
> the tree patches of this series plus a printk to output exit_int_info in
> task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got
> two failures and my VM86 unit test which hung when trying to return from
> VM86. I also ran the kernel that made me aware of the issue initially.
> All debug messages show exit_int_info = 0.

Okay, you are testing on a K8 which has exactly this bug. As I just
found out it is documented as erratum 701. The good news is that this
only happens on K8 and Fam11h, any later AMD processor doesn't have this
bug.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 27.01.2012 14:34, schrieb Joerg Roedel:
> On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote:
>> Am 25.01.2012 17:00, schrieb Joerg Roedel:
> 
>> I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus
>> the tree patches of this series plus a printk to output exit_int_info in
>> task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got
>> two failures and my VM86 unit test which hung when trying to return from
>> VM86. I also ran the kernel that made me aware of the issue initially.
>> All debug messages show exit_int_info = 0.
> 
> Okay, you are testing on a K8 which has exactly this bug. As I just
> found out it is documented as erratum 701. The good news is that this
> only happens on K8 and Fam11h, any later AMD processor doesn't have this
> bug.

Meh. Unless you give me a newer processor, this doesn't really help
me... Doesn't look like there's any way to get a workaround, is there? I
guess I'll have to hack it locally and possibly break other guests with
the hacked module.

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Joerg Roedel]

On Fri, Jan 27, 2012 at 02:55:12PM +0100, Kevin Wolf wrote:
> Am 27.01.2012 14:34, schrieb Joerg Roedel:
> > On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote:
> >> Am 25.01.2012 17:00, schrieb Joerg Roedel:
> > 
> >> I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus
> >> the tree patches of this series plus a printk to output exit_int_info in
> >> task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got
> >> two failures and my VM86 unit test which hung when trying to return from
> >> VM86. I also ran the kernel that made me aware of the issue initially.
> >> All debug messages show exit_int_info = 0.
> > 
> > Okay, you are testing on a K8 which has exactly this bug. As I just
> > found out it is documented as erratum 701. The good news is that this
> > only happens on K8 and Fam11h, any later AMD processor doesn't have this
> > bug.
> 
> Meh. Unless you give me a newer processor, this doesn't really help
> me... Doesn't look like there's any way to get a workaround, is there? I
> guess I'll have to hack it locally and possibly break other guests with
> the hacked module.

No, unfortunatly there is no workaround for this problem. How do you
plan to hack around it?


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf]

Am 27.01.2012 15:17, schrieb Joerg Roedel:
> On Fri, Jan 27, 2012 at 02:55:12PM +0100, Kevin Wolf wrote:
>> Am 27.01.2012 14:34, schrieb Joerg Roedel:
>>> On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote:
>>>> Am 25.01.2012 17:00, schrieb Joerg Roedel:
>>>
>>>> I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus
>>>> the tree patches of this series plus a printk to output exit_int_info in
>>>> task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got
>>>> two failures and my VM86 unit test which hung when trying to return from
>>>> VM86. I also ran the kernel that made me aware of the issue initially.
>>>> All debug messages show exit_int_info = 0.
>>>
>>> Okay, you are testing on a K8 which has exactly this bug. As I just
>>> found out it is documented as erratum 701. The good news is that this
>>> only happens on K8 and Fam11h, any later AMD processor doesn't have this
>>> bug.
>>
>> Meh. Unless you give me a newer processor, this doesn't really help
>> me... Doesn't look like there's any way to get a workaround, is there? I
>> guess I'll have to hack it locally and possibly break other guests with
>> the hacked module.
> 
> No, unfortunatly there is no workaround for this problem. How do you
> plan to hack around it?

I know that my guest only uses iret and exceptions for task switches, so
I think in my case I can assume that any TASK_SWITCH_CALL is really a
TASK_SWITCH_GATE and I don't have to skip an instruction.

Not quite upstreamable, obviously.

Kevin

Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Gleb Natapov]

On Fri, Jan 27, 2012 at 04:02:30PM +0100, Kevin Wolf wrote:
> Am 27.01.2012 15:17, schrieb Joerg Roedel:
> > On Fri, Jan 27, 2012 at 02:55:12PM +0100, Kevin Wolf wrote:
> >> Am 27.01.2012 14:34, schrieb Joerg Roedel:
> >>> On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote:
> >>>> Am 25.01.2012 17:00, schrieb Joerg Roedel:
> >>>
> >>>> I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus
> >>>> the tree patches of this series plus a printk to output exit_int_info in
> >>>> task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got
> >>>> two failures and my VM86 unit test which hung when trying to return from
> >>>> VM86. I also ran the kernel that made me aware of the issue initially.
> >>>> All debug messages show exit_int_info = 0.
> >>>
> >>> Okay, you are testing on a K8 which has exactly this bug. As I just
> >>> found out it is documented as erratum 701. The good news is that this
> >>> only happens on K8 and Fam11h, any later AMD processor doesn't have this
> >>> bug.
> >>
> >> Meh. Unless you give me a newer processor, this doesn't really help
> >> me... Doesn't look like there's any way to get a workaround, is there? I
> >> guess I'll have to hack it locally and possibly break other guests with
> >> the hacked module.
> > 
> > No, unfortunatly there is no workaround for this problem. How do you
> > plan to hack around it?
> 
> I know that my guest only uses iret and exceptions for task switches, so
> I think in my case I can assume that any TASK_SWITCH_CALL is really a
> TASK_SWITCH_GATE and I don't have to skip an instruction.
> 
You still need to know what exception caused task switch. Some of them
require you to skip an instruction.

> Not quite upstreamable, obviously.
> 
> Kevin

--
			Gleb.

[PATCH 2/3] KVM: x86 emulator: VM86 segments must have DPL 3

[Kevin Wolf]

Setting the segment DPL to 0 for at least the VM86 code segment makes
the VM entry fail on VMX.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 arch/x86/kvm/emulate.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 1b98a2c..833969e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1243,6 +1243,8 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		seg_desc.type = 3;
 		seg_desc.p = 1;
 		seg_desc.s = 1;
+		if (ctxt->mode == X86EMUL_MODE_VM86)
+			seg_desc.dpl = 3;
 		goto load;
 	}
 
-- 
1.7.6.5

[PATCH 3/3] KVM: x86 emulator: Allow PM/VM86 switch during task switch

[Kevin Wolf]

Task switches can switch between Protected Mode and VM86. The current
mode must be updated during the task switch emulation so that the new
segment selectors are interpreted correctly and privilege checks
succeed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |   17 +++++++++++++++++
 arch/x86/kvm/x86.c                 |    6 ++++++
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index c8a9cf3..4a21c7d 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -176,6 +176,7 @@ struct x86_emulate_ops {
 	void (*set_idt)(struct x86_emulate_ctxt *ctxt, struct desc_ptr *dt);
 	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
 	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
+	void (*set_rflags)(struct x86_emulate_ctxt *ctxt, ulong val);
 	int (*cpl)(struct x86_emulate_ctxt *ctxt);
 	int (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
 	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 833969e..52fce89 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2273,6 +2273,23 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
 		return emulate_gp(ctxt, 0);
 	ctxt->_eip = tss->eip;
 	ctxt->eflags = tss->eflags | 2;
+
+	/*
+	 * If we're switching between Protected Mode and VM86, we need to make
+	 * sure to update the mode before loading the segment descriptors so
+	 * that the selectors are interpreted correctly.
+	 *
+	 * Need to get it to the vcpu struct immediately because it influences
+	 * the CPL which is checked when loading the segment descriptors.
+	 */
+	if (ctxt->eflags & X86_EFLAGS_VM)
+		ctxt->mode = X86EMUL_MODE_VM86;
+	else
+		ctxt->mode = X86EMUL_MODE_PROT32;
+
+	ctxt->ops->set_rflags(ctxt, ctxt->eflags);
+
+	/* General purpose registers */
 	ctxt->regs[VCPU_REGS_RAX] = tss->eax;
 	ctxt->regs[VCPU_REGS_RCX] = tss->ecx;
 	ctxt->regs[VCPU_REGS_RDX] = tss->edx;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index dc3e945..502b5c3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4040,6 +4040,11 @@ static int emulator_set_cr(struct x86_emulate_ctxt *ctxt, int cr, ulong val)
 	return res;
 }
 
+static void emulator_set_rflags(struct x86_emulate_ctxt *ctxt, ulong val)
+{
+	kvm_set_rflags(emul_to_vcpu(ctxt), val);
+}
+
 static int emulator_get_cpl(struct x86_emulate_ctxt *ctxt)
 {
 	return kvm_x86_ops->get_cpl(emul_to_vcpu(ctxt));
@@ -4199,6 +4204,7 @@ static struct x86_emulate_ops emulate_ops = {
 	.set_idt	     = emulator_set_idt,
 	.get_cr              = emulator_get_cr,
 	.set_cr              = emulator_set_cr,
+	.set_rflags          = emulator_set_rflags,
 	.cpl                 = emulator_get_cpl,
 	.get_dr              = emulator_get_dr,
 	.set_dr              = emulator_set_dr,
-- 
1.7.6.5

Re: [PATCH 3/3] KVM: x86 emulator: Allow PM/VM86 switch during task switch

[Gleb Natapov]

On Mon, Jan 23, 2012 at 05:10:48PM +0100, Kevin Wolf wrote:
> Task switches can switch between Protected Mode and VM86. The current
> mode must be updated during the task switch emulation so that the new
> segment selectors are interpreted correctly and privilege checks
> succeed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  arch/x86/include/asm/kvm_emulate.h |    1 +
>  arch/x86/kvm/emulate.c             |   17 +++++++++++++++++
>  arch/x86/kvm/x86.c                 |    6 ++++++
>  3 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index c8a9cf3..4a21c7d 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -176,6 +176,7 @@ struct x86_emulate_ops {
>  	void (*set_idt)(struct x86_emulate_ctxt *ctxt, struct desc_ptr *dt);
>  	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
>  	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
> +	void (*set_rflags)(struct x86_emulate_ctxt *ctxt, ulong val);
>  	int (*cpl)(struct x86_emulate_ctxt *ctxt);
>  	int (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
>  	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 833969e..52fce89 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2273,6 +2273,23 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
>  		return emulate_gp(ctxt, 0);
>  	ctxt->_eip = tss->eip;
>  	ctxt->eflags = tss->eflags | 2;
> +
> +	/*
> +	 * If we're switching between Protected Mode and VM86, we need to make
> +	 * sure to update the mode before loading the segment descriptors so
> +	 * that the selectors are interpreted correctly.
> +	 *
> +	 * Need to get it to the vcpu struct immediately because it influences
> +	 * the CPL which is checked when loading the segment descriptors.
This is true only for VMX. SVM does not look at rflags. May be instead
of adding new x86_emulate_ops callback we need to get rid of get_cpl()
one and implement CPL checking using emulate.c:get_segment_selector().

> +	 */
> +	if (ctxt->eflags & X86_EFLAGS_VM)
> +		ctxt->mode = X86EMUL_MODE_VM86;
> +	else
> +		ctxt->mode = X86EMUL_MODE_PROT32;
> +
> +	ctxt->ops->set_rflags(ctxt, ctxt->eflags);
> +
> +	/* General purpose registers */
>  	ctxt->regs[VCPU_REGS_RAX] = tss->eax;
>  	ctxt->regs[VCPU_REGS_RCX] = tss->ecx;
>  	ctxt->regs[VCPU_REGS_RDX] = tss->edx;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index dc3e945..502b5c3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4040,6 +4040,11 @@ static int emulator_set_cr(struct x86_emulate_ctxt *ctxt, int cr, ulong val)
>  	return res;
>  }
>  
> +static void emulator_set_rflags(struct x86_emulate_ctxt *ctxt, ulong val)
> +{
> +	kvm_set_rflags(emul_to_vcpu(ctxt), val);
> +}
> +
>  static int emulator_get_cpl(struct x86_emulate_ctxt *ctxt)
>  {
>  	return kvm_x86_ops->get_cpl(emul_to_vcpu(ctxt));
> @@ -4199,6 +4204,7 @@ static struct x86_emulate_ops emulate_ops = {
>  	.set_idt	     = emulator_set_idt,
>  	.get_cr              = emulator_get_cr,
>  	.set_cr              = emulator_set_cr,
> +	.set_rflags          = emulator_set_rflags,
>  	.cpl                 = emulator_get_cpl,
>  	.get_dr              = emulator_get_dr,
>  	.set_dr              = emulator_set_dr,
> -- 
> 1.7.6.5

--
			Gleb.

Re: [PATCH 3/3] KVM: x86 emulator: Allow PM/VM86 switch during task switch

[Kevin Wolf]

Am 24.01.2012 11:57, schrieb Gleb Natapov:
> On Mon, Jan 23, 2012 at 05:10:48PM +0100, Kevin Wolf wrote:
>> Task switches can switch between Protected Mode and VM86. The current
>> mode must be updated during the task switch emulation so that the new
>> segment selectors are interpreted correctly and privilege checks
>> succeed.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_emulate.h |    1 +
>>  arch/x86/kvm/emulate.c             |   17 +++++++++++++++++
>>  arch/x86/kvm/x86.c                 |    6 ++++++
>>  3 files changed, 24 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
>> index c8a9cf3..4a21c7d 100644
>> --- a/arch/x86/include/asm/kvm_emulate.h
>> +++ b/arch/x86/include/asm/kvm_emulate.h
>> @@ -176,6 +176,7 @@ struct x86_emulate_ops {
>>  	void (*set_idt)(struct x86_emulate_ctxt *ctxt, struct desc_ptr *dt);
>>  	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
>>  	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
>> +	void (*set_rflags)(struct x86_emulate_ctxt *ctxt, ulong val);
>>  	int (*cpl)(struct x86_emulate_ctxt *ctxt);
>>  	int (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
>>  	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 833969e..52fce89 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -2273,6 +2273,23 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
>>  		return emulate_gp(ctxt, 0);
>>  	ctxt->_eip = tss->eip;
>>  	ctxt->eflags = tss->eflags | 2;
>> +
>> +	/*
>> +	 * If we're switching between Protected Mode and VM86, we need to make
>> +	 * sure to update the mode before loading the segment descriptors so
>> +	 * that the selectors are interpreted correctly.
>> +	 *
>> +	 * Need to get it to the vcpu struct immediately because it influences
>> +	 * the CPL which is checked when loading the segment descriptors.
> This is true only for VMX. SVM does not look at rflags. May be instead
> of adding new x86_emulate_ops callback we need to get rid of get_cpl()
> one and implement CPL checking using emulate.c:get_segment_selector().

The selector isn't enough for VM86. In most cases
ctxt->ops->get_segment() would work (assuming that the dpl field is
valid there), but it doesn't in task switches before switching the code
segment, which already needs to have the new CPL applied to succeed.

So the only other way I could think of is to pass a flag to
load_segment_descriptor() which says that it shouldn't check any privileges.

Kevin

Re: [PATCH 3/3] KVM: x86 emulator: Allow PM/VM86 switch during task switch

[Gleb Natapov]

On Tue, Jan 24, 2012 at 12:31:48PM +0100, Kevin Wolf wrote:
> Am 24.01.2012 11:57, schrieb Gleb Natapov:
> > On Mon, Jan 23, 2012 at 05:10:48PM +0100, Kevin Wolf wrote:
> >> Task switches can switch between Protected Mode and VM86. The current
> >> mode must be updated during the task switch emulation so that the new
> >> segment selectors are interpreted correctly and privilege checks
> >> succeed.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> ---
> >>  arch/x86/include/asm/kvm_emulate.h |    1 +
> >>  arch/x86/kvm/emulate.c             |   17 +++++++++++++++++
> >>  arch/x86/kvm/x86.c                 |    6 ++++++
> >>  3 files changed, 24 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> >> index c8a9cf3..4a21c7d 100644
> >> --- a/arch/x86/include/asm/kvm_emulate.h
> >> +++ b/arch/x86/include/asm/kvm_emulate.h
> >> @@ -176,6 +176,7 @@ struct x86_emulate_ops {
> >>  	void (*set_idt)(struct x86_emulate_ctxt *ctxt, struct desc_ptr *dt);
> >>  	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
> >>  	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
> >> +	void (*set_rflags)(struct x86_emulate_ctxt *ctxt, ulong val);
> >>  	int (*cpl)(struct x86_emulate_ctxt *ctxt);
> >>  	int (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
> >>  	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
> >> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >> index 833969e..52fce89 100644
> >> --- a/arch/x86/kvm/emulate.c
> >> +++ b/arch/x86/kvm/emulate.c
> >> @@ -2273,6 +2273,23 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
> >>  		return emulate_gp(ctxt, 0);
> >>  	ctxt->_eip = tss->eip;
> >>  	ctxt->eflags = tss->eflags | 2;
> >> +
> >> +	/*
> >> +	 * If we're switching between Protected Mode and VM86, we need to make
> >> +	 * sure to update the mode before loading the segment descriptors so
> >> +	 * that the selectors are interpreted correctly.
> >> +	 *
> >> +	 * Need to get it to the vcpu struct immediately because it influences
> >> +	 * the CPL which is checked when loading the segment descriptors.
> > This is true only for VMX. SVM does not look at rflags. May be instead
> > of adding new x86_emulate_ops callback we need to get rid of get_cpl()
> > one and implement CPL checking using emulate.c:get_segment_selector().
> 
> The selector isn't enough for VM86. In most cases
> ctxt->ops->get_segment() would work (assuming that the dpl field is
> valid there), but it doesn't in task switches before switching the code
> segment, which already needs to have the new CPL applied to succeed.
> 
> So the only other way I could think of is to pass a flag to
> load_segment_descriptor() which says that it shouldn't check any privileges.
> 
What I mean it to have similar logic to what we have in
__vmx_get_cpl(). Something like this:

static int get_cpl(struct x86_emulate_ctxt *ctxt)
{
	if (ctxt->mode == X86EMUL_MODE_REAL)
                return 0;

        if (ctxt->mode != X86EMUL_MODE_PROT64
            && (ctx->eflags & EFLG_VM)) /* if virtual 8086 */
                return 3;

        return get_segment_selector(ctx, VCPU_SREG_CS) & 3;
}

--
			Gleb.

Re: [PATCH 3/3] KVM: x86 emulator: Allow PM/VM86 switch during task switch

[Kevin Wolf]

Am 24.01.2012 12:37, schrieb Gleb Natapov:
> On Tue, Jan 24, 2012 at 12:31:48PM +0100, Kevin Wolf wrote:
>> Am 24.01.2012 11:57, schrieb Gleb Natapov:
>>> On Mon, Jan 23, 2012 at 05:10:48PM +0100, Kevin Wolf wrote:
>>>> Task switches can switch between Protected Mode and VM86. The current
>>>> mode must be updated during the task switch emulation so that the new
>>>> segment selectors are interpreted correctly and privilege checks
>>>> succeed.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>> ---
>>>>  arch/x86/include/asm/kvm_emulate.h |    1 +
>>>>  arch/x86/kvm/emulate.c             |   17 +++++++++++++++++
>>>>  arch/x86/kvm/x86.c                 |    6 ++++++
>>>>  3 files changed, 24 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
>>>> index c8a9cf3..4a21c7d 100644
>>>> --- a/arch/x86/include/asm/kvm_emulate.h
>>>> +++ b/arch/x86/include/asm/kvm_emulate.h
>>>> @@ -176,6 +176,7 @@ struct x86_emulate_ops {
>>>>  	void (*set_idt)(struct x86_emulate_ctxt *ctxt, struct desc_ptr *dt);
>>>>  	ulong (*get_cr)(struct x86_emulate_ctxt *ctxt, int cr);
>>>>  	int (*set_cr)(struct x86_emulate_ctxt *ctxt, int cr, ulong val);
>>>> +	void (*set_rflags)(struct x86_emulate_ctxt *ctxt, ulong val);
>>>>  	int (*cpl)(struct x86_emulate_ctxt *ctxt);
>>>>  	int (*get_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong *dest);
>>>>  	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
>>>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>>>> index 833969e..52fce89 100644
>>>> --- a/arch/x86/kvm/emulate.c
>>>> +++ b/arch/x86/kvm/emulate.c
>>>> @@ -2273,6 +2273,23 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
>>>>  		return emulate_gp(ctxt, 0);
>>>>  	ctxt->_eip = tss->eip;
>>>>  	ctxt->eflags = tss->eflags | 2;
>>>> +
>>>> +	/*
>>>> +	 * If we're switching between Protected Mode and VM86, we need to make
>>>> +	 * sure to update the mode before loading the segment descriptors so
>>>> +	 * that the selectors are interpreted correctly.
>>>> +	 *
>>>> +	 * Need to get it to the vcpu struct immediately because it influences
>>>> +	 * the CPL which is checked when loading the segment descriptors.
>>> This is true only for VMX. SVM does not look at rflags. May be instead
>>> of adding new x86_emulate_ops callback we need to get rid of get_cpl()
>>> one and implement CPL checking using emulate.c:get_segment_selector().
>>
>> The selector isn't enough for VM86. In most cases
>> ctxt->ops->get_segment() would work (assuming that the dpl field is
>> valid there), but it doesn't in task switches before switching the code
>> segment, which already needs to have the new CPL applied to succeed.
>>
>> So the only other way I could think of is to pass a flag to
>> load_segment_descriptor() which says that it shouldn't check any privileges.
>>
> What I mean it to have similar logic to what we have in
> __vmx_get_cpl(). Something like this:
> 
> static int get_cpl(struct x86_emulate_ctxt *ctxt)
> {
> 	if (ctxt->mode == X86EMUL_MODE_REAL)
>                 return 0;
> 
>         if (ctxt->mode != X86EMUL_MODE_PROT64
>             && (ctx->eflags & EFLG_VM)) /* if virtual 8086 */
>                 return 3;
> 
>         return get_segment_selector(ctx, VCPU_SREG_CS) & 3;
> }

Ah, you mean just using ctxt->rflags instead of calling out into x86.c
to update it before the call. Yes, I think that would work.

Kevin