[PATCH v2 0/4] KVM: x86: get CPL from SS.DPL

[PATCH v2 0/4] KVM: x86: get CPL from SS.DPL

[Paolo Bonzini]

Another day, another CPL patch...

It turns out that the simple approach of getting CPL from SS.DPL
broke x86/taskswitch2.flat.  To fix that, already "imagine" that the
CPL is CS.RPL, or 3 for VM86 tasks, while loading segment descriptors
during task switches.  This removes the hack where task switches call
kvm_set_rflags to override the VM flag (patch 2).

While at it, add a new privilege test during task switches that is
missing.

Patch 4 is the same as before.

Paolo Bonzini (4):
  KVM: x86: use new CS.RPL as CPL during task switch
  KVM: x86: drop set_rflags callback
  KVM: x86: check CS.DPL against RPL during task switch
  KVM: x86: get CPL from SS.DPL

 arch/x86/include/asm/kvm_emulate.h |  1 -
 arch/x86/include/asm/kvm_host.h    |  1 -
 arch/x86/kvm/emulate.c             | 63 ++++++++++++++++++++++----------------
 arch/x86/kvm/svm.c                 | 35 +++++++++------------
 arch/x86/kvm/vmx.c                 | 24 +++------------
 arch/x86/kvm/x86.c                 |  6 ----
 6 files changed, 54 insertions(+), 76 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/4] KVM: x86: use new CS.RPL as CPL during task switch

[Paolo Bonzini]

During task switch, all of CS.DPL, CS.RPL, SS.DPL must match (in addition
to all the other requirements) and will be the new CPL.  So far this
worked by carefully setting the CS selector and flag before doing the
task switch; however, this will not work once we get the CPL from SS.DPL:
setting SS itself would fail if the task switch changes the privilege
level.

Temporarily assume that the CPL comes from CS.RPL during task switch
to a protected-mode task.  This is the same approach used in QEMU's
emulation code, which (until version 2.0) manually tracks the CPL.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/emulate.c | 60 +++++++++++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e8a58409b5ac..47e716ef46b7 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1410,11 +1410,11 @@ static int write_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 }
 
 /* Does not support long mode */
-static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
-				   u16 selector, int seg)
+static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
+				     u16 selector, int seg, u8 cpl)
 {
 	struct desc_struct seg_desc, old_desc;
-	u8 dpl, rpl, cpl;
+	u8 dpl, rpl;
 	unsigned err_vec = GP_VECTOR;
 	u32 err_code = 0;
 	bool null_selector = !(selector & ~0x3); /* 0000-0003 are null */
@@ -1442,7 +1442,6 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 	}
 
 	rpl = selector & 3;
-	cpl = ctxt->ops->cpl(ctxt);
 
 	/* NULL selector is not valid for TR, CS and SS (except for long mode) */
 	if ((seg == VCPU_SREG_CS
@@ -1544,6 +1543,13 @@ exception:
 	return X86EMUL_PROPAGATE_FAULT;
 }
 
+static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
+				   u16 selector, int seg)
+{
+	u8 cpl = ctxt->ops->cpl(ctxt);
+	return __load_segment_descriptor(ctxt, selector, seg, cpl);
+}
+
 static void write_register_operand(struct operand *op)
 {
 	/* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
@@ -2405,6 +2411,7 @@ static int load_state_from_tss16(struct x86_emulate_ctxt *ctxt,
 				 struct tss_segment_16 *tss)
 {
 	int ret;
+	u8 cpl;
 
 	ctxt->_eip = tss->ip;
 	ctxt->eflags = tss->flag | 2;
@@ -2427,23 +2434,25 @@ static int load_state_from_tss16(struct x86_emulate_ctxt *ctxt,
 	set_segment_selector(ctxt, tss->ss, VCPU_SREG_SS);
 	set_segment_selector(ctxt, tss->ds, VCPU_SREG_DS);
 
+	cpl = tss->cs & 3;
+
 	/*
 	 * Now load segment descriptors. If fault happens at this stage
 	 * it is handled in a context of new task
 	 */
-	ret = load_segment_descriptor(ctxt, tss->ldt, VCPU_SREG_LDTR);
+	ret = __load_segment_descriptor(ctxt, tss->ldt, VCPU_SREG_LDTR, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES);
+	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS);
+	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS);
+	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS);
+	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
 
@@ -2521,6 +2530,7 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
 				 struct tss_segment_32 *tss)
 {
 	int ret;
+	u8 cpl;
 
 	if (ctxt->ops->set_cr(ctxt, 3, tss->cr3))
 		return emulate_gp(ctxt, 0);
@@ -2539,7 +2549,8 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
 
 	/*
 	 * SDM says that segment selectors are loaded before segment
-	 * descriptors
+	 * descriptors.  This is important because CPL checks will
+	 * use CS.RPL.
 	 */
 	set_segment_selector(ctxt, tss->ldt_selector, VCPU_SREG_LDTR);
 	set_segment_selector(ctxt, tss->es, VCPU_SREG_ES);
@@ -2553,43 +2564,38 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
 	 * 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)
+	if (ctxt->eflags & X86_EFLAGS_VM) {
 		ctxt->mode = X86EMUL_MODE_VM86;
-	else
+		cpl = 3;
+	} else {
 		ctxt->mode = X86EMUL_MODE_PROT32;
-
-	ctxt->ops->set_rflags(ctxt, ctxt->eflags);
+		cpl = tss->cs & 3;
+	}
 
 	/*
 	 * Now load segment descriptors. If fault happenes at this stage
 	 * it is handled in a context of new task
 	 */
-	ret = load_segment_descriptor(ctxt, tss->ldt_selector, VCPU_SREG_LDTR);
+	ret = __load_segment_descriptor(ctxt, tss->ldt_selector, VCPU_SREG_LDTR, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES);
+	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS);
+	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS);
+	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS);
+	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->fs, VCPU_SREG_FS);
+	ret = __load_segment_descriptor(ctxt, tss->fs, VCPU_SREG_FS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = load_segment_descriptor(ctxt, tss->gs, VCPU_SREG_GS);
+	ret = __load_segment_descriptor(ctxt, tss->gs, VCPU_SREG_GS, cpl);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
 
-- 
1.8.3.1

Re: [PATCH v2 1/4] KVM: x86: use new CS.RPL as CPL during task switch

[Paolo Bonzini]

Il 15/05/2014 18:51, Paolo Bonzini ha scritto:
> During task switch, all of CS.DPL, CS.RPL, SS.DPL must match (in addition
> to all the other requirements) and will be the new CPL.  So far this
> worked by carefully setting the CS selector and flag before doing the

s/flag/EFLAGS/

> task switch; however, this will not work once we get the CPL from SS.DPL:
> setting SS itself would fail if the task switch changes the privilege
> level.

More precisely, before patch 4 in this series setting CS.selector would 
already change the CPL.  After it, you actually have to set the full 
segment descriptor cache to change the CPL, so we cannot use 
ctxt->ops->cpl(ctxt) to retrieve the CPL during a task switch.  The 
check that fails without this patch is that SS.DPL must be == CPL, and 
the failure happens because ctxt->ops->cpl(ctxt) is the *old* SS.DPL.

Paolo

> Temporarily assume that the CPL comes from CS.RPL during task switch
> to a protected-mode task.  This is the same approach used in QEMU's
> emulation code, which (until version 2.0) manually tracks the CPL.

Re: [PATCH v2 1/4] KVM: x86: use new CS.RPL as CPL during task switch

[Marcelo Tosatti]

On Thu, May 15, 2014 at 06:51:28PM +0200, Paolo Bonzini wrote:
> During task switch, all of CS.DPL, CS.RPL, SS.DPL must match (in addition
> to all the other requirements) and will be the new CPL.  So far this
> worked by carefully setting the CS selector and flag before doing the
> task switch; however, this will not work once we get the CPL from SS.DPL:
> setting SS itself would fail if the task switch changes the privilege
> level.
> 
> Temporarily assume that the CPL comes from CS.RPL during task switch
> to a protected-mode task.  This is the same approach used in QEMU's
> emulation code, which (until version 2.0) manually tracks the CPL.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

[PATCH v2 2/4] KVM: x86: drop set_rflags callback

[Paolo Bonzini]

Not needed anymore now that the CPL is computed directly
by the task switch code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_emulate.h | 1 -
 arch/x86/kvm/x86.c                 | 6 ------
 2 files changed, 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 24ec1216596e..a04fe4eb237d 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -189,7 +189,6 @@ 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/x86.c b/arch/x86/kvm/x86.c
index fb313fc896dd..57eac309c22e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4646,11 +4646,6 @@ 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));
@@ -4835,7 +4830,6 @@ static const 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.8.3.1

[PATCH v2 3/4] KVM: x86: check CS.DPL against RPL during task switch

[Paolo Bonzini]

Table 7-1 of the SDM mentions a check that the code segment's
DPL must match the selector's RPL.  This was not done by KVM,
fix it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/emulate.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 47e716ef46b7..2fa7ab069817 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1411,7 +1411,7 @@ static int write_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 
 /* Does not support long mode */
 static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
-				     u16 selector, int seg, u8 cpl)
+				     u16 selector, int seg, u8 cpl, bool in_task_switch)
 {
 	struct desc_struct seg_desc, old_desc;
 	u8 dpl, rpl;
@@ -1486,6 +1486,9 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 			goto exception;
 		break;
 	case VCPU_SREG_CS:
+		if (in_task_switch && rpl != dpl)
+			goto exception;
+
 		if (!(seg_desc.type & 8))
 			goto exception;
 
@@ -1547,7 +1550,7 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 				   u16 selector, int seg)
 {
 	u8 cpl = ctxt->ops->cpl(ctxt);
-	return __load_segment_descriptor(ctxt, selector, seg, cpl);
+	return __load_segment_descriptor(ctxt, selector, seg, cpl, false);
 }
 
 static void write_register_operand(struct operand *op)
@@ -2440,19 +2443,19 @@ static int load_state_from_tss16(struct x86_emulate_ctxt *ctxt,
 	 * Now load segment descriptors. If fault happens at this stage
 	 * it is handled in a context of new task
 	 */
-	ret = __load_segment_descriptor(ctxt, tss->ldt, VCPU_SREG_LDTR, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->ldt, VCPU_SREG_LDTR, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
 
@@ -2577,25 +2580,25 @@ static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
 	 * Now load segment descriptors. If fault happenes at this stage
 	 * it is handled in a context of new task
 	 */
-	ret = __load_segment_descriptor(ctxt, tss->ldt_selector, VCPU_SREG_LDTR, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->ldt_selector, VCPU_SREG_LDTR, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->es, VCPU_SREG_ES, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->cs, VCPU_SREG_CS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->ss, VCPU_SREG_SS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->ds, VCPU_SREG_DS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->fs, VCPU_SREG_FS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->fs, VCPU_SREG_FS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
-	ret = __load_segment_descriptor(ctxt, tss->gs, VCPU_SREG_GS, cpl);
+	ret = __load_segment_descriptor(ctxt, tss->gs, VCPU_SREG_GS, cpl, true);
 	if (ret != X86EMUL_CONTINUE)
 		return ret;
 
-- 
1.8.3.1

Re: [PATCH v2 3/4] KVM: x86: check CS.DPL against RPL during task switch

[Marcelo Tosatti]

On Thu, May 15, 2014 at 06:51:30PM +0200, Paolo Bonzini wrote:
> Table 7-1 of the SDM mentions a check that the code segment's
> DPL must match the selector's RPL.  This was not done by KVM,
> fix it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Exception number is incorrect, however not introduced by this patchset.

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

[PATCH v2 4/4] KVM: x86: get CPL from SS.DPL

[Paolo Bonzini]

CS.RPL is not equal to the CPL in the few instructions between
setting CR0.PE and reloading CS.  And CS.DPL is also not equal
to the CPL for conforming code segments.

However, SS.DPL *is* always equal to the CPL except for the weird
case of SYSRET on AMD processors, which sets SS.DPL=SS.RPL from the
value in the STAR MSR, but force CPL=3 (Intel instead forces
SS.DPL=SS.RPL=CPL=3).

So this patch:

- modifies SVM to update the CPL from SS.DPL rather than CS.RPL;
the above case with SYSRET is not broken further, and the way
to fix it would be to pass the CPL to userspace and back

- modifies VMX to always return the CPL from SS.DPL (except
forcing it to 0 if we are emulating real mode via vm86 mode;
in vm86 mode all DPLs have to be 3, but real mode does allow
privileged instructions).  It also removes the CPL cache,
which becomes a duplicate of the SS access rights cache.

This fixes doing KVM_IOCTL_SET_SREGS exactly after setting
CR0.PE=1 but before CS has been reloaded.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/svm.c              | 35 ++++++++++++++---------------------
 arch/x86/kvm/vmx.c              | 24 ++++--------------------
 3 files changed, 18 insertions(+), 42 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e21aee98a5c2..49314155b66c 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -130,7 +130,6 @@ enum kvm_reg_ex {
 	VCPU_EXREG_PDPTR = NR_VCPU_REGS,
 	VCPU_EXREG_CR3,
 	VCPU_EXREG_RFLAGS,
-	VCPU_EXREG_CPL,
 	VCPU_EXREG_SEGMENTS,
 };
 
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 0b7d58d0c5fb..ec8366c5cfea 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -1338,21 +1338,6 @@ 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;
@@ -1360,11 +1345,12 @@ static unsigned long svm_get_rflags(struct kvm_vcpu *vcpu)
 
 static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
 {
-	unsigned long old_rflags = to_svm(vcpu)->vmcb->save.rflags;
-
+       /*
+        * Any change of EFLAGS.VM is accompained by a reload of SS
+        * (caused by either a task switch or an inter-privilege IRET),
+        * so we do not need to update the CPL here.
+        */
 	to_svm(vcpu)->vmcb->save.rflags = rflags;
-	if ((old_rflags ^ rflags) & X86_EFLAGS_VM)
-		svm_update_cpl(vcpu);
 }
 
 static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
@@ -1631,8 +1617,15 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
 		s->attrib |= (var->db & 1) << SVM_SELECTOR_DB_SHIFT;
 		s->attrib |= (var->g & 1) << SVM_SELECTOR_G_SHIFT;
 	}
-	if (seg == VCPU_SREG_CS)
-		svm_update_cpl(vcpu);
+
+	/*
+	 * This is always accurate, except if SYSRET returned to a segment
+	 * with SS.DPL != 3.  Intel does not have this quirk, and always
+	 * forces SS.DPL to 3 on sysret, so we ignore that case; fixing it
+	 * would entail passing the CPL to userspace and back.
+	 */
+	if (seg == VCPU_SREG_SS)
+		svm->vmcb->save.cpl = (s->attrib >> SVM_SELECTOR_DPL_SHIFT) & 3;
 
 	mark_dirty(svm->vmcb, VMCB_SEG);
 }
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6f7463f53ed9..a267108403f5 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -414,7 +414,6 @@ struct vcpu_vmx {
 	struct kvm_vcpu       vcpu;
 	unsigned long         host_rsp;
 	u8                    fail;
-	u8                    cpl;
 	bool                  nmi_known_unmasked;
 	u32                   exit_intr_info;
 	u32                   idt_vectoring_info;
@@ -3150,10 +3149,6 @@ static void enter_pmode(struct kvm_vcpu *vcpu)
 	fix_pmode_seg(vcpu, VCPU_SREG_DS, &vmx->rmode.segs[VCPU_SREG_DS]);
 	fix_pmode_seg(vcpu, VCPU_SREG_FS, &vmx->rmode.segs[VCPU_SREG_FS]);
 	fix_pmode_seg(vcpu, VCPU_SREG_GS, &vmx->rmode.segs[VCPU_SREG_GS]);
-
-	/* CPL is always 0 when CPU enters protected mode */
-	__set_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
-	vmx->cpl = 0;
 }
 
 static void fix_rmode_seg(int seg, struct kvm_segment *save)
@@ -3555,22 +3550,14 @@ static int vmx_get_cpl(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
-	if (!is_protmode(vcpu))
+	if (unlikely(vmx->rmode.vm86_active))
 		return 0;
-
-	if (!is_long_mode(vcpu)
-	    && (kvm_get_rflags(vcpu) & X86_EFLAGS_VM)) /* if virtual 8086 */
-		return 3;
-
-	if (!test_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail)) {
-		__set_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
-		vmx->cpl = vmx_read_guest_seg_selector(vmx, VCPU_SREG_CS) & 3;
+	else {
+		int ar = vmx_read_guest_seg_ar(vmx, VCPU_SREG_SS);
+		return AR_DPL(ar);
 	}
-
-	return vmx->cpl;
 }
 
-
 static u32 vmx_segment_access_rights(struct kvm_segment *var)
 {
 	u32 ar;
@@ -3598,8 +3585,6 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
 	const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
 
 	vmx_segment_cache_clear(vmx);
-	if (seg == VCPU_SREG_CS)
-		__clear_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
 
 	if (vmx->rmode.vm86_active && seg != VCPU_SREG_LDTR) {
 		vmx->rmode.segs[seg] = *var;
@@ -7471,7 +7456,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
 	vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP)
 				  | (1 << VCPU_EXREG_RFLAGS)
-				  | (1 << VCPU_EXREG_CPL)
 				  | (1 << VCPU_EXREG_PDPTR)
 				  | (1 << VCPU_EXREG_SEGMENTS)
 				  | (1 << VCPU_EXREG_CR3));
-- 
1.8.3.1

Re: [PATCH v2 4/4] KVM: x86: get CPL from SS.DPL

[Marcelo Tosatti]

On Thu, May 15, 2014 at 06:51:31PM +0200, Paolo Bonzini wrote:
> CS.RPL is not equal to the CPL in the few instructions between
> setting CR0.PE and reloading CS.  And CS.DPL is also not equal
> to the CPL for conforming code segments.
> 
> However, SS.DPL *is* always equal to the CPL except for the weird
> case of SYSRET on AMD processors, which sets SS.DPL=SS.RPL from the
> value in the STAR MSR, but force CPL=3 (Intel instead forces
> SS.DPL=SS.RPL=CPL=3).
> 
> So this patch:
> 
> - modifies SVM to update the CPL from SS.DPL rather than CS.RPL;
> the above case with SYSRET is not broken further, and the way
> to fix it would be to pass the CPL to userspace and back
> 
> - modifies VMX to always return the CPL from SS.DPL (except
> forcing it to 0 if we are emulating real mode via vm86 mode;
> in vm86 mode all DPLs have to be 3, but real mode does allow
> privileged instructions).  It also removes the CPL cache,
> which becomes a duplicate of the SS access rights cache.
> 
> This fixes doing KVM_IOCTL_SET_SREGS exactly after setting
> CR0.PE=1 but before CS has been reloaded.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>


Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

Re: [PATCH v2 4/4] KVM: x86: get CPL from SS.DPL

[Wei Huang]

> CS.RPL is not equal to the CPL in the few instructions between
> setting CR0.PE and reloading CS.  And CS.DPL is also not equal
> to the CPL for conforming code segments.

Out of my curiousity, could you elaborate the problem of this
CPL gap window, such as breaking any VMs or tests? From Linux kernel
code, it seems kernel enables CR0.PE and immediately ljmpl to
CS. This windows is very small and I am curious how severely it could
be.

>
> However, SS.DPL *is* always equal to the CPL except for the weird

Is this specified anywhere in SDM as a requirement for x86 OS? If so,
maybe provide a pointer to support this.

> case of SYSRET on AMD processors, which sets SS.DPL=SS.RPL from the
> value in the STAR MSR, but force CPL=3 (Intel instead forces
> SS.DPL=SS.RPL=CPL=3).

Thinking out loud here... Should we force SYSRET SS.RPL to be 3 when
VM updates STAR MSR? Following the same thought, does it make sense to
check (and force) SS.DPL==3 when STAR MSR is being updated. Will
forcing SYSRET SS.DPL=3 break any OS? I think any reasonable OS would
probably sets SS.RPL=SS.DPL=3.

>
> So this patch:
>
> - modifies SVM to update the CPL from SS.DPL rather than CS.RPL;
> the above case with SYSRET is not broken further, and the way
> to fix it would be to pass the CPL to userspace and back
>
> - modifies VMX to always return the CPL from SS.DPL (except
> forcing it to 0 if we are emulating real mode via vm86 mode;
> in vm86 mode all DPLs have to be 3, but real mode does allow
> privileged instructions).  It also removes the CPL cache,
> which becomes a duplicate of the SS access rights cache.
>
> This fixes doing KVM_IOCTL_SET_SREGS exactly after setting

nit-picking: s/KVM_IOCTL_SET_SREGS/KVM_SET_SREGS IOCTL/, to
match with IOCTL function name exactly.

> CR0.PE=1 but before CS has been reloaded.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 -
>  arch/x86/kvm/svm.c              | 35 ++++++++++++++---------------------
>  arch/x86/kvm/vmx.c              | 24 ++++--------------------
>  3 files changed, 18 insertions(+), 42 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index e21aee98a5c2..49314155b66c 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -130,7 +130,6 @@ enum kvm_reg_ex {
>   VCPU_EXREG_PDPTR = NR_VCPU_REGS,
>   VCPU_EXREG_CR3,
>   VCPU_EXREG_RFLAGS,
> - VCPU_EXREG_CPL,
>   VCPU_EXREG_SEGMENTS,
>  };
>
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 0b7d58d0c5fb..ec8366c5cfea 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -1338,21 +1338,6 @@ 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;
> @@ -1360,11 +1345,12 @@ static unsigned long svm_get_rflags(struct
> kvm_vcpu *vcpu)
>
>  static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>  {
> - unsigned long old_rflags = to_svm(vcpu)->vmcb->save.rflags;
> -
> +       /*
> +        * Any change of EFLAGS.VM is accompained by a reload of SS
> +        * (caused by either a task switch or an inter-privilege IRET),
> +        * so we do not need to update the CPL here.
> +        */
>   to_svm(vcpu)->vmcb->save.rflags = rflags;
> - if ((old_rflags ^ rflags) & X86_EFLAGS_VM)
> - svm_update_cpl(vcpu);
>  }
>
>  static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
> @@ -1631,8 +1617,15 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
>   s->attrib |= (var->db & 1) << SVM_SELECTOR_DB_SHIFT;
>   s->attrib |= (var->g & 1) << SVM_SELECTOR_G_SHIFT;
>   }
> - if (seg == VCPU_SREG_CS)
> - svm_update_cpl(vcpu);
> +
> + /*
> + * This is always accurate, except if SYSRET returned to a segment
> + * with SS.DPL != 3.  Intel does not have this quirk, and always
> + * forces SS.DPL to 3 on sysret, so we ignore that case; fixing it
> + * would entail passing the CPL to userspace and back.
> + */
> + if (seg == VCPU_SREG_SS)
> + svm->vmcb->save.cpl = (s->attrib >> SVM_SELECTOR_DPL_SHIFT) & 3;
>
>   mark_dirty(svm->vmcb, VMCB_SEG);
>  }
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 6f7463f53ed9..a267108403f5 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -414,7 +414,6 @@ struct vcpu_vmx {
>   struct kvm_vcpu       vcpu;
>   unsigned long         host_rsp;
>   u8                    fail;
> - u8                    cpl;
>   bool                  nmi_known_unmasked;
>   u32                   exit_intr_info;
>   u32                   idt_vectoring_info;
> @@ -3150,10 +3149,6 @@ static void enter_pmode(struct kvm_vcpu *vcpu)
>   fix_pmode_seg(vcpu, VCPU_SREG_DS, &vmx->rmode.segs[VCPU_SREG_DS]);
>   fix_pmode_seg(vcpu, VCPU_SREG_FS, &vmx->rmode.segs[VCPU_SREG_FS]);
>   fix_pmode_seg(vcpu, VCPU_SREG_GS, &vmx->rmode.segs[VCPU_SREG_GS]);
> -
> - /* CPL is always 0 when CPU enters protected mode */
> - __set_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
> - vmx->cpl = 0;
>  }
>
>  static void fix_rmode_seg(int seg, struct kvm_segment *save)
> @@ -3555,22 +3550,14 @@ static int vmx_get_cpl(struct kvm_vcpu *vcpu)
>  {
>   struct vcpu_vmx *vmx = to_vmx(vcpu);
>
> - if (!is_protmode(vcpu))
> + if (unlikely(vmx->rmode.vm86_active))
>   return 0;
> -
> - if (!is_long_mode(vcpu)
> -    && (kvm_get_rflags(vcpu) & X86_EFLAGS_VM)) /* if virtual 8086 */
> - return 3;
> -
> - if (!test_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail)) {
> - __set_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
> - vmx->cpl = vmx_read_guest_seg_selector(vmx, VCPU_SREG_CS) & 3;
> + else {
> + int ar = vmx_read_guest_seg_ar(vmx, VCPU_SREG_SS);
> + return AR_DPL(ar);
>   }
> -
> - return vmx->cpl;
>  }
>
> -
>  static u32 vmx_segment_access_rights(struct kvm_segment *var)
>  {
>   u32 ar;
> @@ -3598,8 +3585,6 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
>   const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
>
>   vmx_segment_cache_clear(vmx);
> - if (seg == VCPU_SREG_CS)
> - __clear_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
>
>   if (vmx->rmode.vm86_active && seg != VCPU_SREG_LDTR) {
>   vmx->rmode.segs[seg] = *var;
> @@ -7471,7 +7456,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>
>   vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP)
>    | (1 << VCPU_EXREG_RFLAGS)
> -  | (1 << VCPU_EXREG_CPL)
>    | (1 << VCPU_EXREG_PDPTR)
>    | (1 << VCPU_EXREG_SEGMENTS)
>    | (1 << VCPU_EXREG_CR3));

Re: [PATCH v2 4/4] KVM: x86: get CPL from SS.DPL

[Paolo Bonzini]

Il 26/05/2014 01:21, Wei Huang ha scritto:
>> CS.RPL is not equal to the CPL in the few instructions between
>> setting CR0.PE and reloading CS.  And CS.DPL is also not equal
>> to the CPL for conforming code segments.
>
> Out of my curiousity, could you elaborate the problem of this
> CPL gap window, such as breaking any VMs or tests? From Linux kernel
> code, it seems kernel enables CR0.PE and immediately ljmpl to
> CS. This windows is very small and I am curious how severely it could
> be.

It is almost guaranteed to happen if you run iPXE and in the meanwhile 
kick QEMU continuously:

# qemu-system-x86_64 -monitor unix:/tmp/m1,server,nowait -net ... &
# yes 'info cpus' | nc -U /tmp/m1

>> However, SS.DPL *is* always equal to the CPL except for the weird
>
> Is this specified anywhere in SDM as a requirement for x86 OS? If so,
> maybe provide a pointer to support this.

In the case of the Intel manuals, it mentions in several places that 
SS.DPL=CPL.  All the mentions are in the VMX sections of the manual, 
though I've found non-Intel material saying that system-management mode 
also used SS.DPL as the CPL:

* "SS.DPL corresponds to the logical processor’s current privilege level 
(CPL)" (footnote in 26.3.1.5 Checks on Guest Non-Register State).

* "SS.DPL is always loaded from the SS access-rights field. This will be 
the current privilege level (CPL) after the VM entry completes" 
(26.3.2.2 Loading Guest Segment Registers and Descriptor-Table Registers)

* "VMX-critical state [...] consists of the following: (1) SS.DPL (the 
current privilege level);" (34.14.1 Default Treatment of SMI delivery 
[in VMX mode]).

Instead, AMD says that "the SS segment base, limit and attributes are 
not modified" by sysret.  It almost looks as if AMD processors never use 
SS.DPL; almost because searching "SS.attr" in the AMD manuals shows that 
the processor does write to SS.attr sometimes.  In the SVM 
documentation, it says "The processor reads the current privilege level 
from the CPL field in the VMCB, not from SS.DPL.  However, SS.DPL should 
match the CPL field" and sneakily leaves out what happens if they do not 
match...

>> case of SYSRET on AMD processors, which sets SS.DPL=SS.RPL from the
>> value in the STAR MSR, but force CPL=3 (Intel instead forces
>> SS.DPL=SS.RPL=CPL=3).
>
> Thinking out loud here... Should we force SYSRET SS.RPL to be 3 when
> VM updates STAR MSR? Following the same thought, does it make sense to
> check (and force) SS.DPL==3 when STAR MSR is being updated. Will
> forcing SYSRET SS.DPL=3 break any OS? I think any reasonable OS would
> probably sets SS.RPL=SS.DPL=3.

Yes, I wondered in fact how much the AMD behavior is a bug.

We could emulate Intel behavior on AMD by shadowing the STAR MSR; the 
guest reads the intended SS.DPL and SS.RPL but the processor actually 
always runs with bits 49-48 of STAR set to 3.  This should ensure that 
CPL=SS.DPL always even on AMD.  I'm not sure if this has any worth though...

Paolo

>>
>> So this patch:
>>
>> - modifies SVM to update the CPL from SS.DPL rather than CS.RPL;
>> the above case with SYSRET is not broken further, and the way
>> to fix it would be to pass the CPL to userspace and back
>>
>> - modifies VMX to always return the CPL from SS.DPL (except
>> forcing it to 0 if we are emulating real mode via vm86 mode;
>> in vm86 mode all DPLs have to be 3, but real mode does allow
>> privileged instructions).  It also removes the CPL cache,
>> which becomes a duplicate of the SS access rights cache.
>>
>> This fixes doing KVM_IOCTL_SET_SREGS exactly after setting
>
> nit-picking: s/KVM_IOCTL_SET_SREGS/KVM_SET_SREGS IOCTL/, to
> match with IOCTL function name exactly.
>
>> CR0.PE=1 but before CS has been reloaded.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |  1 -
>>  arch/x86/kvm/svm.c              | 35 ++++++++++++++---------------------
>>  arch/x86/kvm/vmx.c              | 24 ++++--------------------
>>  3 files changed, 18 insertions(+), 42 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index e21aee98a5c2..49314155b66c 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -130,7 +130,6 @@ enum kvm_reg_ex {
>>   VCPU_EXREG_PDPTR = NR_VCPU_REGS,
>>   VCPU_EXREG_CR3,
>>   VCPU_EXREG_RFLAGS,
>> - VCPU_EXREG_CPL,
>>   VCPU_EXREG_SEGMENTS,
>>  };
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index 0b7d58d0c5fb..ec8366c5cfea 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -1338,21 +1338,6 @@ 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;
>> @@ -1360,11 +1345,12 @@ static unsigned long svm_get_rflags(struct
>> kvm_vcpu *vcpu)
>>
>>  static void svm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags)
>>  {
>> - unsigned long old_rflags = to_svm(vcpu)->vmcb->save.rflags;
>> -
>> +       /*
>> +        * Any change of EFLAGS.VM is accompained by a reload of SS
>> +        * (caused by either a task switch or an inter-privilege IRET),
>> +        * so we do not need to update the CPL here.
>> +        */
>>   to_svm(vcpu)->vmcb->save.rflags = rflags;
>> - if ((old_rflags ^ rflags) & X86_EFLAGS_VM)
>> - svm_update_cpl(vcpu);
>>  }
>>
>>  static void svm_cache_reg(struct kvm_vcpu *vcpu, enum kvm_reg reg)
>> @@ -1631,8 +1617,15 @@ static void svm_set_segment(struct kvm_vcpu *vcpu,
>>   s->attrib |= (var->db & 1) << SVM_SELECTOR_DB_SHIFT;
>>   s->attrib |= (var->g & 1) << SVM_SELECTOR_G_SHIFT;
>>   }
>> - if (seg == VCPU_SREG_CS)
>> - svm_update_cpl(vcpu);
>> +
>> + /*
>> + * This is always accurate, except if SYSRET returned to a segment
>> + * with SS.DPL != 3.  Intel does not have this quirk, and always
>> + * forces SS.DPL to 3 on sysret, so we ignore that case; fixing it
>> + * would entail passing the CPL to userspace and back.
>> + */
>> + if (seg == VCPU_SREG_SS)
>> + svm->vmcb->save.cpl = (s->attrib >> SVM_SELECTOR_DPL_SHIFT) & 3;
>>
>>   mark_dirty(svm->vmcb, VMCB_SEG);
>>  }
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 6f7463f53ed9..a267108403f5 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -414,7 +414,6 @@ struct vcpu_vmx {
>>   struct kvm_vcpu       vcpu;
>>   unsigned long         host_rsp;
>>   u8                    fail;
>> - u8                    cpl;
>>   bool                  nmi_known_unmasked;
>>   u32                   exit_intr_info;
>>   u32                   idt_vectoring_info;
>> @@ -3150,10 +3149,6 @@ static void enter_pmode(struct kvm_vcpu *vcpu)
>>   fix_pmode_seg(vcpu, VCPU_SREG_DS, &vmx->rmode.segs[VCPU_SREG_DS]);
>>   fix_pmode_seg(vcpu, VCPU_SREG_FS, &vmx->rmode.segs[VCPU_SREG_FS]);
>>   fix_pmode_seg(vcpu, VCPU_SREG_GS, &vmx->rmode.segs[VCPU_SREG_GS]);
>> -
>> - /* CPL is always 0 when CPU enters protected mode */
>> - __set_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
>> - vmx->cpl = 0;
>>  }
>>
>>  static void fix_rmode_seg(int seg, struct kvm_segment *save)
>> @@ -3555,22 +3550,14 @@ static int vmx_get_cpl(struct kvm_vcpu *vcpu)
>>  {
>>   struct vcpu_vmx *vmx = to_vmx(vcpu);
>>
>> - if (!is_protmode(vcpu))
>> + if (unlikely(vmx->rmode.vm86_active))
>>   return 0;
>> -
>> - if (!is_long_mode(vcpu)
>> -    && (kvm_get_rflags(vcpu) & X86_EFLAGS_VM)) /* if virtual 8086 */
>> - return 3;
>> -
>> - if (!test_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail)) {
>> - __set_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
>> - vmx->cpl = vmx_read_guest_seg_selector(vmx, VCPU_SREG_CS) & 3;
>> + else {
>> + int ar = vmx_read_guest_seg_ar(vmx, VCPU_SREG_SS);
>> + return AR_DPL(ar);
>>   }
>> -
>> - return vmx->cpl;
>>  }
>>
>> -
>>  static u32 vmx_segment_access_rights(struct kvm_segment *var)
>>  {
>>   u32 ar;
>> @@ -3598,8 +3585,6 @@ static void vmx_set_segment(struct kvm_vcpu *vcpu,
>>   const struct kvm_vmx_segment_field *sf = &kvm_vmx_segment_fields[seg];
>>
>>   vmx_segment_cache_clear(vmx);
>> - if (seg == VCPU_SREG_CS)
>> - __clear_bit(VCPU_EXREG_CPL, (ulong *)&vcpu->arch.regs_avail);
>>
>>   if (vmx->rmode.vm86_active && seg != VCPU_SREG_LDTR) {
>>   vmx->rmode.segs[seg] = *var;
>> @@ -7471,7 +7456,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>>
>>   vcpu->arch.regs_avail = ~((1 << VCPU_REGS_RIP) | (1 << VCPU_REGS_RSP)
>>    | (1 << VCPU_EXREG_RFLAGS)
>> -  | (1 << VCPU_EXREG_CPL)
>>    | (1 << VCPU_EXREG_PDPTR)
>>    | (1 << VCPU_EXREG_SEGMENTS)
>>    | (1 << VCPU_EXREG_CR3));

Re: [PATCH v2 4/4] KVM: x86: get CPL from SS.DPL

[Wei Huang]

>>
>>
>> Is this specified anywhere in SDM as a requirement for x86 OS? If so,
>> maybe provide a pointer to support this.
>
>
> In the case of the Intel manuals, it mentions in several places that
> SS.DPL=CPL.  All the mentions are in the VMX sections of the manual, though
> I've found non-Intel material saying that system-management mode also used
> SS.DPL as the CPL:
>
> * "SS.DPL corresponds to the logical processor’s current privilege level
> (CPL)" (footnote in 26.3.1.5 Checks on Guest Non-Register State).
>
> * "SS.DPL is always loaded from the SS access-rights field. This will be the
> current privilege level (CPL) after the VM entry completes" (26.3.2.2
> Loading Guest Segment Registers and Descriptor-Table Registers)
>
> * "VMX-critical state [...] consists of the following: (1) SS.DPL (the
> current privilege level);" (34.14.1 Default Treatment of SMI delivery [in
> VMX mode]).
>
> Instead, AMD says that "the SS segment base, limit and attributes are not
> modified" by sysret.  It almost looks as if AMD processors never use SS.DPL;
> almost because searching "SS.attr" in the AMD manuals shows that the
> processor does write to SS.attr sometimes.  In the SVM documentation, it
> says "The processor reads the current privilege level from the CPL field in
> the VMCB, not from SS.DPL.  However, SS.DPL should match the CPL field" and
> sneakily leaves out what happens if they do not match...

My guess is that SS.DPL==CPL will fail during VMRUN. This can be
quickly tested by slightly
tweaking VMCB content of a regular VM.

>
>
>>> case of SYSRET on AMD processors, which sets SS.DPL=SS.RPL from the
>>> value in the STAR MSR, but force CPL=3 (Intel instead forces
>>> SS.DPL=SS.RPL=CPL=3).
>>
>>
>> Thinking out loud here... Should we force SYSRET SS.RPL to be 3 when
>> VM updates STAR MSR? Following the same thought, does it make sense to
>> check (and force) SS.DPL==3 when STAR MSR is being updated. Will
>> forcing SYSRET SS.DPL=3 break any OS? I think any reasonable OS would
>> probably sets SS.RPL=SS.DPL=3.
>
>
> Yes, I wondered in fact how much the AMD behavior is a bug.
>
> We could emulate Intel behavior on AMD by shadowing the STAR MSR; the guest
> reads the intended SS.DPL and SS.RPL but the processor actually always runs
> with bits 49-48 of STAR set to 3.  This should ensure that CPL=SS.DPL always
> even on AMD.  I'm not sure if this has any worth though...

When SS.DPL != CPL for a VM, the worst case without STAR emulation
proposed above
is the crash of the VM, which it deserves. So I think we are fine here.

>
> Paolo
>
>