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

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

[Kevin Wolf]

Kevin Wolf (4):
  KVM: x86 emulator: Fix task switch privilege checks
  KVM: x86 emulator: VM86 segments must have DPL 3
  KVM: SVM: Fix CPL updates
  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             |   75 ++++++++++++++++++++++++++++++++---
 arch/x86/kvm/svm.c                 |   25 ++++++++++--
 arch/x86/kvm/vmx.c                 |    8 ++-
 arch/x86/kvm/x86.c                 |   12 ++++-
 6 files changed, 107 insertions(+), 20 deletions(-)

-- 
1.7.6.5

[PATCH v3 1/4] 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.

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             |   53 +++++++++++++++++++++++++++++++-----
 arch/x86/kvm/svm.c                 |    5 +++-
 arch/x86/kvm/vmx.c                 |    8 +++--
 arch/x86/kvm/x86.c                 |    6 ++--
 6 files changed, 61 insertions(+), 17 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..7097ca9 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;
@@ -2372,12 +2388,35 @@ 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. jmp/call/int to task gate: Check against DPL of the task gate
+	 * 2. Exception/IRQ/iret: No check is performed
+	 * 3. jmp/call to TSS: Check agains DPL of the TSS
+	 */
+	if (reason == TASK_SWITCH_GATE) {
+		if (idt_index != -1) {
+			/* Software interrupts */
+			struct kvm_desc_struct task_gate_desc;
+			int dpl;
+
+			ret = read_interrupt_descriptor(ctxt, idt_index,
+							&task_gate_desc);
+			if (ret != X86EMUL_CONTINUE)
+				return ret;
+
+			dpl = task_gate_desc.dpl;
+			if ((tss_selector & 3) > dpl || ops->cpl(ctxt) > dpl)
+				return emulate_gp(ctxt, (idt_index << 3) | 0x2);
+		}
+	} else if (reason != TASK_SWITCH_IRET) {
+		int dpl = next_tss_desc.dpl;
+		if ((tss_selector & 3) > dpl || ops->cpl(ctxt) > dpl)
+			return emulate_gp(ctxt, tss_selector);
 	}
 
+
 	desc_limit = desc_limit_scaled(&next_tss_desc);
 	if (!next_tss_desc.p ||
 	    ((desc_limit < 0x67 && (next_tss_desc.type & 8)) ||
@@ -2430,7 +2469,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 +2477,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..6a977c1 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2730,7 +2730,10 @@ 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,
+	if (int_type != SVM_EXITINTINFO_TYPE_SOFT)
+		int_vec = -1;
+
+	if (kvm_task_switch(&svm->vcpu, tss_selector, int_vec, 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..a335170 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4672,9 +4672,10 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
 	bool has_error_code = false;
 	u32 error_code = 0;
 	u16 tss_selector;
-	int reason, type, idt_v;
+	int reason, type, idt_v, idt_index;
 
 	idt_v = (vmx->idt_vectoring_info & VECTORING_INFO_VALID_MASK);
+	idt_index = (vmx->idt_vectoring_info & VECTORING_INFO_VECTOR_MASK);
 	type = (vmx->idt_vectoring_info & VECTORING_INFO_TYPE_MASK);
 
 	exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
@@ -4712,8 +4713,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_index : -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

[PATCH v3 2/4] 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 7097ca9..144a203 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 v3 3/4] KVM: SVM: Fix CPL updates

[Kevin Wolf]

Keep CPL at 0 in real mode and at 3 in VM86. In protected/long mode, use
RPL rather than DPL of the code segment.

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 6a977c1..4124a7e 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1263,6 +1263,21 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
 		wrmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
 }
 
+static void svm_update_cpl(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+	int cpl;
+
+	if (!is_protmode(vcpu))
+		cpl = 0;
+	else if (svm->vmcb->save.rflags & X86_EFLAGS_VM)
+		cpl = 3;
+	else
+		cpl = svm->vmcb->save.cs.selector & 0x3;
+
+	svm->vmcb->save.cpl = cpl;
+}
+
 static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
 {
 	return to_svm(vcpu)->vmcb->save.rflags;
@@ -1538,9 +1553,7 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
 		s->attrib |= (var->g & 1) << SVM_SELECTOR_G_SHIFT;
 	}
 	if (seg == VCPU_SREG_CS)
-		svm->vmcb->save.cpl
-			= (svm->vmcb->save.cs.attrib
-			   >> SVM_SELECTOR_DPL_SHIFT) & 3;
+		svm_update_cpl(vcpu);
 
 	mark_dirty(svm->vmcb, VMCB_SEG);
 }
-- 
1.7.6.5

[PATCH v3 4/4] 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.

In order to let privilege checks succeed, rflags needs to be updated in
the vcpu struct as this causes a CPL update.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |   20 ++++++++++++++++++++
 arch/x86/kvm/svm.c                 |    1 +
 arch/x86/kvm/x86.c                 |    6 ++++++
 4 files changed, 28 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 144a203..a9fc21d 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2273,6 +2273,8 @@ 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;
+
+	/* General purpose registers */
 	ctxt->regs[VCPU_REGS_RAX] = tss->eax;
 	ctxt->regs[VCPU_REGS_RCX] = tss->ecx;
 	ctxt->regs[VCPU_REGS_RDX] = tss->edx;
@@ -2295,6 +2297,24 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
 	set_segment_selector(ctxt, tss->gs, VCPU_SREG_GS);
 
 	/*
+	 * 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 rflags to the vcpu struct immediately because it
+	 * influences the CPL which is checked at least when loading the segment
+	 * descriptors and when pushing an error code to the new kernel stack.
+	 *
+	 * TODO Introduce a separate ctxt->ops->set_cpl callback
+	 */
+	if (ctxt->eflags & X86_EFLAGS_VM)
+		ctxt->mode = X86EMUL_MODE_VM86;
+	else
+		ctxt->mode = X86EMUL_MODE_PROT32;
+
+	ctxt->ops->set_rflags(ctxt, ctxt->eflags);
+
+	/*
 	 * Now load segment descriptors. If fault happenes at this stage
 	 * it is handled in a context of new task
 	 */
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 4124a7e..38d5ef3 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1286,6 +1286,7 @@ static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
 static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 {
 	to_svm(vcpu)->vmcb->save.rflags = rflags;
+	svm_update_cpl(vcpu);
 }
 
 static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
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 v3 3/4] KVM: SVM: Fix CPL updates

[Gleb Natapov]

On Fri, Feb 03, 2012 at 07:29:24PM +0100, Kevin Wolf wrote:
> Keep CPL at 0 in real mode and at 3 in VM86. In protected/long mode, use
> RPL rather than DPL of the code segment.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  arch/x86/kvm/svm.c |   19 ++++++++++++++++---
>  1 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 6a977c1..4124a7e 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1263,6 +1263,21 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
>  		wrmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
>  }
>  
> +static void svm_update_cpl(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	int cpl;
> +
> +	if (!is_protmode(vcpu))
> +		cpl = 0;
> +	else if (svm->vmcb->save.rflags & X86_EFLAGS_VM)
> +		cpl = 3;
> +	else
> +		cpl = svm->vmcb->save.cs.selector & 0x3;
> +
> +	svm->vmcb->save.cpl = cpl;
> +}
> +
As you probably know already I think cpl should be updated in
svm_get_rflags() too. With current patch restoring CS segment
register before rflags register after migration may cause cpl
to get wrong value for instance.

>  static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
>  {
>  	return to_svm(vcpu)->vmcb->save.rflags;
> @@ -1538,9 +1553,7 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
>  		s->attrib |= (var->g & 1) << SVM_SELECTOR_G_SHIFT;
>  	}
>  	if (seg == VCPU_SREG_CS)
> -		svm->vmcb->save.cpl
> -			= (svm->vmcb->save.cs.attrib
> -			   >> SVM_SELECTOR_DPL_SHIFT) & 3;
> +		svm_update_cpl(vcpu);
>  
>  	mark_dirty(svm->vmcb, VMCB_SEG);
>  }
> -- 
> 1.7.6.5

--
			Gleb.

Re: [PATCH v3 3/4] KVM: SVM: Fix CPL updates

[Kevin Wolf]

Am 05.02.2012 12:16, schrieb Gleb Natapov:
> On Fri, Feb 03, 2012 at 07:29:24PM +0100, Kevin Wolf wrote:
>> Keep CPL at 0 in real mode and at 3 in VM86. In protected/long mode, use
>> RPL rather than DPL of the code segment.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  arch/x86/kvm/svm.c |   19 ++++++++++++++++---
>>  1 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 6a977c1..4124a7e 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1263,6 +1263,21 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
>>  		wrmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
>>  }
>>  
>> +static void svm_update_cpl(struct kvm_vcpu *vcpu)
>> +{
>> +	struct vcpu_svm *svm = to_svm(vcpu);
>> +	int cpl;
>> +
>> +	if (!is_protmode(vcpu))
>> +		cpl = 0;
>> +	else if (svm->vmcb->save.rflags & X86_EFLAGS_VM)
>> +		cpl = 3;
>> +	else
>> +		cpl = svm->vmcb->save.cs.selector & 0x3;
>> +
>> +	svm->vmcb->save.cpl = cpl;
>> +}
>> +
> As you probably know already I think cpl should be updated in
> svm_get_rflags() too. With current patch restoring CS segment
> register before rflags register after migration may cause cpl
> to get wrong value for instance.

I think you mean set_rflags rather than get_rflags?

Patch 4 adds this with the set_rflags emulator callback.

Kevin

Re: [PATCH v3 3/4] KVM: SVM: Fix CPL updates

[Gleb Natapov]

On Mon, Feb 06, 2012 at 10:18:35AM +0100, Kevin Wolf wrote:
> Am 05.02.2012 12:16, schrieb Gleb Natapov:
> > On Fri, Feb 03, 2012 at 07:29:24PM +0100, Kevin Wolf wrote:
> >> Keep CPL at 0 in real mode and at 3 in VM86. In protected/long mode, use
> >> RPL rather than DPL of the code segment.
> >>
> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> >> ---
> >>  arch/x86/kvm/svm.c |   19 ++++++++++++++++---
> >>  1 files changed, 16 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> >> index 6a977c1..4124a7e 100644
> >> --- a/arch/x86/kvm/svm.c
> >> +++ b/arch/x86/kvm/svm.c
> >> @@ -1263,6 +1263,21 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
> >>  		wrmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
> >>  }
> >>  
> >> +static void svm_update_cpl(struct kvm_vcpu *vcpu)
> >> +{
> >> +	struct vcpu_svm *svm = to_svm(vcpu);
> >> +	int cpl;
> >> +
> >> +	if (!is_protmode(vcpu))
> >> +		cpl = 0;
> >> +	else if (svm->vmcb->save.rflags & X86_EFLAGS_VM)
> >> +		cpl = 3;
> >> +	else
> >> +		cpl = svm->vmcb->save.cs.selector & 0x3;
> >> +
> >> +	svm->vmcb->save.cpl = cpl;
> >> +}
> >> +
> > As you probably know already I think cpl should be updated in
> > svm_get_rflags() too. With current patch restoring CS segment
> > register before rflags register after migration may cause cpl
> > to get wrong value for instance.
> 
> I think you mean set_rflags rather than get_rflags?
> 
Uh, yes.

> Patch 4 adds this with the set_rflags emulator callback.
> 
Sorry, missed that, I expected it to be in this patch for some reason.

I think calling svm_update_cpl() from set_rflags() is incorrect though.
svm_update_cpl() checks cr0 so if guest does "mov 1, %cr0; popf" and
popf happens to be emulated it will change cpl to cs&3 which is
incorrect.

--
			Gleb.

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

[Avi Kivity]

On 02/03/2012 08:29 PM, 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.
>
> In order to let privilege checks succeed, rflags needs to be updated in
> the vcpu struct as this causes a CPL update.
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 4124a7e..38d5ef3 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1286,6 +1286,7 @@ static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
>  static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>  {
>  	to_svm(vcpu)->vmcb->save.rflags = rflags;
> +	svm_update_cpl(vcpu);
>  }

This can trigger the (cs&3)!=0 && (cr0&1) != 0 problem, right after we
enter protected mode.

What this code does is slave the cpl to other x86 state (copying vmx),
whereas in reality it is separate state (with independent existence from
the mov cr0 instruction until the next far jump...).

We can fix this with

    if ((save.rflags ^ rflags) & EFLAGS_VM)
        svm_update_cpl(vcpu)

but that leaves us with an unupdated cpl after live migration (well, it
will update after loading segment registers).  Both are bad, but I think
my version is less bad - live migration is broken anyway in this window,
since we don't save the cpl.



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH v3 3/4] KVM: SVM: Fix CPL updates

[Kevin Wolf]

Am 06.02.2012 10:57, schrieb Gleb Natapov:
> On Mon, Feb 06, 2012 at 10:18:35AM +0100, Kevin Wolf wrote:
>> Am 05.02.2012 12:16, schrieb Gleb Natapov:
>>> On Fri, Feb 03, 2012 at 07:29:24PM +0100, Kevin Wolf wrote:
>>>> Keep CPL at 0 in real mode and at 3 in VM86. In protected/long mode, use
>>>> RPL rather than DPL of the code segment.
>>>>
>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>>> ---
>>>>  arch/x86/kvm/svm.c |   19 ++++++++++++++++---
>>>>  1 files changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>>>> index 6a977c1..4124a7e 100644
>>>> --- a/arch/x86/kvm/svm.c
>>>> +++ b/arch/x86/kvm/svm.c
>>>> @@ -1263,6 +1263,21 @@ static void svm_vcpu_put(struct kvm_vcpu *vcpu)
>>>>  		wrmsrl(host_save_user_msrs[i], svm->host_user_msrs[i]);
>>>>  }
>>>>  
>>>> +static void svm_update_cpl(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +	struct vcpu_svm *svm = to_svm(vcpu);
>>>> +	int cpl;
>>>> +
>>>> +	if (!is_protmode(vcpu))
>>>> +		cpl = 0;
>>>> +	else if (svm->vmcb->save.rflags & X86_EFLAGS_VM)
>>>> +		cpl = 3;
>>>> +	else
>>>> +		cpl = svm->vmcb->save.cs.selector & 0x3;
>>>> +
>>>> +	svm->vmcb->save.cpl = cpl;
>>>> +}
>>>> +
>>> As you probably know already I think cpl should be updated in
>>> svm_get_rflags() too. With current patch restoring CS segment
>>> register before rflags register after migration may cause cpl
>>> to get wrong value for instance.
>>
>> I think you mean set_rflags rather than get_rflags?
>>
> Uh, yes.
> 
>> Patch 4 adds this with the set_rflags emulator callback.
>>
> Sorry, missed that, I expected it to be in this patch for some reason.
> 
> I think calling svm_update_cpl() from set_rflags() is incorrect though.
> svm_update_cpl() checks cr0 so if guest does "mov 1, %cr0; popf" and
> popf happens to be emulated it will change cpl to cs&3 which is
> incorrect.

Good point. The easy, but IMHO somewhat hackish way to fix it is to only
call svm_update_cpl() if the VM flag has changed.

For the real thing, we'd have to know whether we are in the middle of a
mode switch, i.e. whether the segment selector is a protected mode or
real mode selector. I don't think we have this information, do we?

Kevin

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

[Kevin Wolf]

Am 06.02.2012 11:32, schrieb Avi Kivity:
> On 02/03/2012 08:29 PM, 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.
>>
>> In order to let privilege checks succeed, rflags needs to be updated in
>> the vcpu struct as this causes a CPL update.
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 4124a7e..38d5ef3 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1286,6 +1286,7 @@ static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
>>  static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>>  {
>>  	to_svm(vcpu)->vmcb->save.rflags = rflags;
>> +	svm_update_cpl(vcpu);
>>  }
> 
> This can trigger the (cs&3)!=0 && (cr0&1) != 0 problem, right after we
> enter protected mode.
> 
> What this code does is slave the cpl to other x86 state (copying vmx),
> whereas in reality it is separate state (with independent existence from
> the mov cr0 instruction until the next far jump...).

Yes, as discussed in v2, eventually we'll want to call set_cpl() rather
than set_rflags() during the task switch (I left a TODO comment there).
Then the not quite correct update in svm_set_rflags can be removed again.

> We can fix this with
> 
>     if ((save.rflags ^ rflags) & EFLAGS_VM)
>         svm_update_cpl(vcpu)
> 
> but that leaves us with an unupdated cpl after live migration (well, it
> will update after loading segment registers).  Both are bad, but I think
> my version is less bad - live migration is broken anyway in this window,
> since we don't save the cpl.

Right, it just leaves broken what is already broken. If the rest of the
series is okay now, I'll resend with a check if rflags.VM has changed.

Kevin