KVM: x86: Fix TSS size check for 16-bit tasks

PATCH In review Jan Kiszka 3 participants 8 messages Mar 22, 2010 – Mar 23, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2010-03-22 10:29 UTC #0 lore raw

PATCH 1/1 KVM: x86: Fix TSS size check for 16-bit tasks

A 16-bit TSS is only 44 bytes long. So make sure to test for the correct size on task switch.

Signed-off-by Jan Kiszka <[email protected]>

---

This should be stable material as well. I can provide a patch that applies on .32 and .33, or what will be the procedure?

 arch/x86/kvm/emulate.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

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

index 266576c..f529f75 100644
@@ -2355,6 +2355,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 	u16 old_tss_sel = ops->get_segment_selector(VCPU_SREG_TR, ctxt->vcpu);
 	ulong old_tss_base =
 		get_cached_descriptor_base(ctxt, ops, VCPU_SREG_TR);
+	u32 desc_limit;
 
 	/* FIXME: old_tss_base == ~0 ? */

@@ -2375,7 +2376,10 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 		}
 	}
 
-	if (!next_tss_desc.p || desc_limit_scaled(&next_tss_desc) < 0x67) {
+	desc_limit = desc_limit_scaled(&next_tss_desc);
+	if (!next_tss_desc.p ||
+	    ((desc_limit < 0x67 && (next_tss_desc.type & 8)) ||
+	     desc_limit < 0x2c)) {
 		kvm_queue_exception_e(ctxt->vcpu, TS_VECTOR,
 				      tss_selector & 0xfffc);
 		return X86EMUL_PROPAGATE_FAULT;

Avi Kivity

2010-03-23 10:25 UTC #1 lore raw

Quoted 7 lines · Jan Kiszka

A 16-bit TSS is only 44 bytes long. So make sure to test for the correct size on task switch.
This should be stable material as well. I can provide a patch that applies on .32 and .33, or what will be the procedure?

I'd like to drop the Cc: stable and maintain stable queues explicitly (in kvm-updates/2.6.3[23]). I'll fast-forward these to current -stable, please send patches against them. These branches will be autotested before submission, a step that is missing in the current scheme of things.

Review

Quoted 7 lines

(quoted diff) +4 -1

-	if (!next_tss_desc.p || desc_limit_scaled(&next_tss_desc)<  0x67) {
+	desc_limit = desc_limit_scaled(&next_tss_desc);
+	if (!next_tss_desc.p ||
+	    ((desc_limit<  0x67&&  (next_tss_desc.type&  8)) ||
+	     desc_limit<  0x2c)) {

A 44-byte TSS has a limit of 43 (just like a 4GB segment has a limit of 0xffffffff), so there is an off-by-one here.

Signature · 1 line

error compiling committee.c: too many arguments to function

Avi Kivity

2010-03-23 10:42 UTC #2 lore raw

Quoted 6 lines · Avi Kivity

Quoted 2 lines
This should be stable material as well. I can provide a patch that applies on .32 and .33, or what will be the procedure?
I'd like to drop the Cc: stable and maintain stable queues explicitly (in kvm-updates/2.6.3[23]). I'll fast-forward these to current -stable, please send patches against them. These branches will be
autotested before submission, a step that is missing in the current scheme of things.

kvm.git now has these branches updated.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2010-03-23 12:15 UTC #3 lore raw

Quoted 4 lines · Avi Kivity

Quoted 3 lines · Avi Kivity
Quoted 2 lines
This should be stable material as well. I can provide a patch that applies on .32 and .33, or what will be the procedure?

Quoted 8 lines

Quoted 5 lines
I'd like to drop the Cc: stable and maintain stable queues explicitly (in kvm-updates/2.6.3[23]). I'll fast-forward these to current -stable, please send patches against them. These branches will be autotested before submission, a step that is missing in the current scheme of things.
kvm.git now has these branches updated.

6c1535244cc930a0307ea0708a2063e4f4b34158 ("Add KVM_CAP_X86_ROBUST_SINGLESTEP") in the 2.6.33 branch is damaged. "Update instruction length on intercepted BP" is missing in the 2.6.32 queue, but Greg should have queued this up already.

Jan

Jan Kiszka

2010-03-23 11:24 UTC #4 lore raw

In reply to Avi Kivity

Quoted 22 lines · Avi Kivity

Quoted 7 lines · Jan Kiszka
A 16-bit TSS is only 44 bytes long. So make sure to test for the correct size on task switch.
This should be stable material as well. I can provide a patch that applies on .32 and .33, or what will be the procedure?
I'd like to drop the Cc: stable and maintain stable queues explicitly (in kvm-updates/2.6.3[23]). I'll fast-forward these to current -stable, please send patches against them. These branches will be autotested before submission, a step that is missing in the current scheme of things.
Review
Quoted 7 lines
(quoted diff) +4 -1
-    if (!next_tss_desc.p || desc_limit_scaled(&next_tss_desc)<  0x67) {
+    desc_limit = desc_limit_scaled(&next_tss_desc);
+    if (!next_tss_desc.p ||
+        ((desc_limit<  0x67&&  (next_tss_desc.type&  8)) ||
+         desc_limit<  0x2c)) {

Quoted 3 lines

A 44-byte TSS has a limit of 43 (just like a 4GB segment has a limit of 0xffffffff), so there is an off-by-one here.

Right - you just found an (harmless) off-by-one in our legacy OS as well (I blindly copied its limit).

Jan

Avi Kivity

2010-03-23 12:25 UTC #5 lore raw

Quoted 9 lines · Jan Kiszka

Quoted 5 lines
A 44-byte TSS has a limit of 43 (just like a 4GB segment has a limit of 0xffffffff), so there is an off-by-one here.
Right - you just found an (harmless) off-by-one in our legacy OS as well (I blindly copied its limit).

It's a very common error.

Signature · 1 line

error compiling committee.c: too many arguments to function

Jan Kiszka

2010-03-23 11:25 UTC #6 lore raw

PATCH v2 1/1 KVM: x86: Fix TSS size check for 16-bit tasks

In reply to Jan Kiszka

From: Jan Kiszka <[email protected]>

A 16-bit TSS is only 44 bytes long. So make sure to test for the correct size on task switch.

Signed-off-by Jan Kiszka <[email protected]>

---

Changes in v2:
- fixed off-by-one in 16-bit limit

 arch/x86/kvm/emulate.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

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

index 266576c..ab3fff5 100644
@@ -2355,6 +2355,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 	u16 old_tss_sel = ops->get_segment_selector(VCPU_SREG_TR, ctxt->vcpu);
 	ulong old_tss_base =
 		get_cached_descriptor_base(ctxt, ops, VCPU_SREG_TR);
+	u32 desc_limit;
 
 	/* FIXME: old_tss_base == ~0 ? */

@@ -2375,7 +2376,10 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
 		}
 	}
 
-	if (!next_tss_desc.p || desc_limit_scaled(&next_tss_desc) < 0x67) {
+	desc_limit = desc_limit_scaled(&next_tss_desc);
+	if (!next_tss_desc.p ||
+	    ((desc_limit < 0x67 && (next_tss_desc.type & 8)) ||
+	     desc_limit < 0x2b)) {
 		kvm_queue_exception_e(ctxt->vcpu, TS_VECTOR,
 				      tss_selector & 0xfffc);
 		return X86EMUL_PROPAGATE_FAULT;

Avi Kivity

Applied

2010-03-23 12:35 UTC #7 lore raw

Quoted 5 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>
A 16-bit TSS is only 44 bytes long. So make sure to test for the correct size on task switch.

Applied, thanks.

Signature · 1 line

error compiling committee.c: too many arguments to function