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

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

[Jan Kiszka]

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 <jan.kiszka@siemens.com>
---

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(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 266576c..f529f75 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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;

Re: [PATCH] KVM: x86: Fix TSS size check for 16-bit tasks

[Avi Kivity]

On 03/22/2010 12:29 PM, Jan Kiszka wrote:
> 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.

>
> -	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.

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

Re: [PATCH] KVM: x86: Fix TSS size check for 16-bit tasks

[Avi Kivity]

On 03/23/2010 12:25 PM, Avi Kivity wrote:
>> 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.

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

Re: [PATCH] KVM: x86: Fix TSS size check for 16-bit tasks

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1150 bytes --]

Avi Kivity wrote:
> On 03/22/2010 12:29 PM, Jan Kiszka wrote:
>> 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.
> 
>>
>> -    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.
> 

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

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[PATCH v2] KVM: x86: Fix TSS size check for 16-bit tasks

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---

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

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

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 266576c..ab3fff5 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -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;

Re: [PATCH] KVM: x86: Fix TSS size check for 16-bit tasks

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

Avi Kivity wrote:
> On 03/23/2010 12:25 PM, Avi Kivity wrote:
>>> 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.
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH] KVM: x86: Fix TSS size check for 16-bit tasks

[Avi Kivity]

On 03/23/2010 01:21 PM, Jan Kiszka wrote:
>
>> 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.

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

Re: [PATCH v2] KVM: x86: Fix TSS size check for 16-bit tasks

[Avi Kivity]

On 03/23/2010 01:25 PM, Jan Kiszka wrote:
> From: Jan Kiszka<jan.kiszka@siemens.com>
>
> A 16-bit TSS is only 44 bytes long. So make sure to test for the correct
> size on task switch.
>    

Applied, thanks.

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