Regression problem with commit 5045b46803

Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch. Unfortunately, is causes some of my tests that run well on bare-metal to fail.

Although this check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions.

Thus, I recommend on reverting commit 5045b46803, or alternatively rechecking task-switch behavior on bare-metal.

Il 13/08/2014 19:14, Nadav Amit ha scritto:

Quoted 4 lines

Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch. Unfortunately, is causes some of my tests that run well on bare-metal to fail.

Quoted 6 lines

Although this check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions.

Thus, I recommend on reverting commit 5045b46803, or alternatively rechecking task-switch behavior on bare-metal.

Can you provide a kvm-unit-tests test for this?

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 6 lines

Although this check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions.

Thus, I recommend on reverting commit 5045b46803, or alternatively rechecking task-switch behavior on bare-metal.

Can you provide a kvm-unit-tests test for this?

I need to put time into writing one. However, the unit test would prove nothing. The question is how bare-metal behaves. According to what I see, it does not perform this check.

Il 17/08/2014 08:23, Nadav Amit ha scritto:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

Although this check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions.

Thus, I recommend on reverting commit 5045b46803, or alternatively rechecking task-switch behavior on bare-metal.

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Can you provide a kvm-unit-tests test for this?

The question is how bare-metal behaves. According to what I see, it does not perform this check.

Yes, but I still prefer to have a test, in case I find some time to fix kvm-unit-tests to work on bare metal (or on Bochs).

There are already a few task switch tests (32-bit only).

Can you please assist in running the existing task-switch tests first?

In x86_64 the task-switch tests are skipped. If I configure the tests to use the i386 architecture, the build fails. Even after fixing the compilation errors, I don’t manage to get the task-switch tests to pass.

Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch. This is a wrong check, and this test introduces a test in which cs.dpl != cs.rpl. To do so, it configures tss.cs to be conforming with rpl=3 and dpl=0. Since the cpl after calling is 3, it does not make any prints in the callee.

Signed-off-by Nadav Amit <[email protected]>

---
 x86/taskswitch2.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

x86/taskswitch2.c +0 -0

index 92fc941..d96853f 100644

x86/taskswitch2.c +5 -0

+	test_count++;
+	asm volatile ("iret");
+	goto start;
+}
+
 void test_kernel_mode_int()
 {
 	unsigned int res;

@@ -201,6 +211,18 @@ void test_kernel_mode_int()
 	asm volatile ("ljmp $" xstr(TSS_INTR) ", $0xf4f4f4f4");
 	printf("Jump back succeeded\n");
 	report("ljmp", test_count == 1);

x86/taskswitch2.c +2 -0

+	asm volatile("lcall $" xstr(TSS_INTR) ", $0xf4f4f4f4");
+	report("lcall when cs.rpl != cs.dpl", test_count == 1);
 }
 
 void test_vm86_switch(void)

Commit 5045b46803 added a check that cs.dpl equals cs.rpl during task-switch. This is a wrong check, and this patch introduces a test in which cs.dpl != cs.rpl. To do so, it configures tss.cs to be conforming with rpl=3 and dpl=0. Since the cpl after calling is 3, it does not make any prints in the callee.

Signed-off-by Nadav Amit <[email protected]>

---
 x86/taskswitch2.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

x86/taskswitch2.c +9 -0

index 92fc941..1fe833e 100644
@@ -7,6 +7,7 @@
 
 #define MAIN_TSS_SEL (FIRST_SPARE_SEL + 0)
 #define VM86_TSS_SEL (FIRST_SPARE_SEL + 8)
+#define CONFORM_CS_SEL  (FIRST_SPARE_SEL + 16)
 
 static volatile int test_count;
 static volatile unsigned int test_divider;

@@ -102,6 +103,14 @@ start:
 	goto start;
 }
 
+static void user_tss(void)
+{
+start:
+	test_count++;
+	asm volatile ("iret");
+	goto start;
+}
+
 void test_kernel_mode_int()
 {
 	unsigned int res;

x86/taskswitch2.c +9 -0

+
+	tss_intr.cs = CONFORM_CS_SEL | 3;
+	tss_intr.ds = tss_intr.gs = tss_intr.fs = tss_intr.ss = USER_DS;
+	set_gdt_entry(CONFORM_CS_SEL, 0, 0xffffffff, 0x9f, 0xc0);
+	asm volatile("lcall $" xstr(TSS_INTR) ", $0xf4f4f4f4");
+	report("lcall with cs.rpl != cs.dpl", test_count == 1);
+}
+
 int main()
 {
 	setup_vm();

@@ -256,6 +278,7 @@ int main()
 
 	test_kernel_mode_int();
 	test_vm86_switch();
+	test_conforming_switch();
 
 	return report_summary();
 }

Il 19/08/2014 15:04, Nadav Amit ha scritto:

x86/taskswitch2.c void test_vm86_switch(void) · line 264

Quoted 6 lines

x86/taskswitch2.c +5 -0

+void test_conforming_switch(void)
+{
+	/* test lcall with conforming segment, cs.dpl != cs.rpl */
+	test_count = 0;
+	set_intr_task_gate(0, user_tss);

No need to use set_intr_task_gate, since the IDT is not involved here.

       tss_intr.eip = (u32)user_tss;

is enough.

I fixed this up and applied the patch. Thanks!

I know, but all the other ‘call' tests did. If there is an error, at least it should be consistent. ;-)

This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160. Although the cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions. As it causes some tests, which pass on bare-metal, to fail - it should be reverted.

Il 17/08/2014 21:32, Nadav Amit ha scritto:

Quoted 5 lines

This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160. Although the cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions. As it causes some tests, which pass on bare-metal, to fail - it should be reverted.

Right. However, I think reverting the patch is too big a hammer. We still need in_task_switch to raise TS_VECTOR instead of GP_VECTOR, so I propose instead something like:

arch/x86/kvm/emulate.c +1 -1

index 56657b0bb3bb..cd230b035514 100644
@@ -1468,7 +1468,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		return ret;
 
 	err_code = selector & 0xfffc;
-	err_vec = GP_VECTOR;
+	err_vec = in_task_switch ? TS_VECTOR : GP_VECTOR;
 
 	/* can't load system descriptor into segment selector */
 	if (seg <= VCPU_SREG_GS && !seg_desc.s)

Il 17/08/2014 23:09, Paolo Bonzini ha scritto:

Quoted 22 lines

Il 17/08/2014 21:32, Nadav Amit ha scritto:

Quoted 5 lines

This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160. Although the cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions. As it causes some tests, which pass on bare-metal, to fail - it should be reverted.

Right. However, I think reverting the patch is too big a hammer. We still need in_task_switch to raise TS_VECTOR instead of GP_VECTOR, so I propose instead something like:

arch/x86/kvm/emulate.c +1 -1

index 56657b0bb3bb..cd230b035514 100644
@@ -1468,7 +1468,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		return ret;
 
 	err_code = selector & 0xfffc;
-	err_vec = GP_VECTOR;
+	err_vec = in_task_switch ? TS_VECTOR : GP_VECTOR;
 

arch/x86/kvm/emulate.c static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt, · line 1496

Quoted 12 lines

arch/x86/kvm/emulate.c +0 -3

 	/* can't load system descriptor into segment selector */
 	if (seg <= VCPU_SREG_GS && !seg_desc.s)

@@ -1491,9 +1491,6 @@ 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;
 

Also, what about the rpl > cpl test below, for non-conforming code segments? It is not mentioned in table 6-6 either.

Quoted 2 lines

Quoted 2 lines

Also, what about the rpl > cpl test below, for non-conforming code segments? It is not mentioned in table 6-6 either.

As far as I understand, after task-switch cpl = cs.rpl. This is how the load_state_from_tss32 does it, and follows SDM 7.3, Task Switching:
"The new task begins executing at the privilege level specified in the CPL field of the CS register, which is loaded from the TSS.” As a result, this condition can never occur during task-switch.

Do I miss anything?

Il 17/08/2014 21:32, Nadav Amit ha scritto:

Quoted 5 lines

This reverts commit 5045b468037dfe1c848827ce10e99d87f5669160. Although the cs.dpl=cs.rpl check is mentioned in table 7-1 of the SDM as causing a #TSS exception, it is not mentioned in table 6-6 that lists "invalid TSS conditions" which cause #TSS exceptions. As it causes some tests, which pass on bare-metal, to fail - it should be reverted.

Right. However, I think reverting the patch is too big a hammer. We still need in_task_switch to raise TS_VECTOR instead of GP_VECTOR, so I propose instead something like:

arch/x86/kvm/emulate.c +1 -4

index 56657b0bb3bb..cd230b035514 100644
@@ -1468,7 +1468,7 @@ static int __load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
 		return ret;
 
 	err_code = selector & 0xfffc;
-	err_vec = GP_VECTOR;
+	err_vec = in_task_switch ? TS_VECTOR : GP_VECTOR;
 
 	/* can't load system descriptor into segment selector */
 	if (seg <= VCPU_SREG_GS && !seg_desc.s)

@@ -1491,9 +1491,6 @@ 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;
 

either in a single patch or as two separate patch. I'll test this against your test case and repost (not before Tuesday).

I missed the TS_VECTOR thing. Yes, in that case, full revert makes no sense. I’m in no hurry with this patch, and I posted it during your vacation only because it is a recent bug. Anyhow, you may want to look at another patch I sent, "KVM: x86: Avoid emulating instructions on #UD mistakenly”, ASAP. It fixes a bug that was introduced into 3.17-RC1 and is visible in userspace.