2014-04-07 15:38:51

by Nadav Amit

[permalink] [raw]
Subject: [PATCH] KVM: x86: Fix CR3 and LDT sel should not be saved in TSS

According to Intel specifications, only general purpose registers and segment
selectors should are saved in the old TSS during 32-bit task-switch.

Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/kvm/emulate.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 205b17e..0dec502 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2496,7 +2496,7 @@ static int task_switch_16(struct x86_emulate_ctxt *ctxt,
static void save_state_to_tss32(struct x86_emulate_ctxt *ctxt,
struct tss_segment_32 *tss)
{
- tss->cr3 = ctxt->ops->get_cr(ctxt, 3);
+ /* CR3 and ldt selector are not saved intentionally */
tss->eip = ctxt->_eip;
tss->eflags = ctxt->eflags;
tss->eax = reg_read(ctxt, VCPU_REGS_RAX);
@@ -2514,7 +2514,6 @@ static void save_state_to_tss32(struct x86_emulate_ctxt *ctxt,
tss->ds = get_segment_selector(ctxt, VCPU_SREG_DS);
tss->fs = get_segment_selector(ctxt, VCPU_SREG_FS);
tss->gs = get_segment_selector(ctxt, VCPU_SREG_GS);
- tss->ldt_selector = get_segment_selector(ctxt, VCPU_SREG_LDTR);
}

static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
@@ -2604,6 +2603,8 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
struct tss_segment_32 tss_seg;
int ret;
u32 new_tss_base = get_desc_base(new_desc);
+ u32 eip_offset = offsetof(struct tss_segment_32, eip);
+ u32 ldt_sel_offset = offsetof(struct tss_segment_32, ldt_selector);

ret = ops->read_std(ctxt, old_tss_base, &tss_seg, sizeof tss_seg,
&ctxt->exception);
@@ -2613,8 +2614,9 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,

save_state_to_tss32(ctxt, &tss_seg);

- ret = ops->write_std(ctxt, old_tss_base, &tss_seg, sizeof tss_seg,
- &ctxt->exception);
+ /* Only GP registers and segment selectors are saved */
+ ret = ops->write_std(ctxt, old_tss_base + eip_offset, &tss_seg.eip,
+ ldt_sel_offset - eip_offset, &ctxt->exception);
if (ret != X86EMUL_CONTINUE)
/* FIXME: need to provide precise fault address */
return ret;
--
1.7.10.4


2014-04-16 19:21:15

by Marcelo Tosatti

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: Fix CR3 and LDT sel should not be saved in TSS

On Mon, Apr 07, 2014 at 06:37:47PM +0300, Nadav Amit wrote:
> According to Intel specifications, only general purpose registers and segment
> selectors should are saved in the old TSS during 32-bit task-switch.

should be

> Signed-off-by: Nadav Amit <[email protected]>
> ---
> arch/x86/kvm/emulate.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 205b17e..0dec502 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2496,7 +2496,7 @@ static int task_switch_16(struct x86_emulate_ctxt *ctxt,
> static void save_state_to_tss32(struct x86_emulate_ctxt *ctxt,
> struct tss_segment_32 *tss)
> {
> - tss->cr3 = ctxt->ops->get_cr(ctxt, 3);
> + /* CR3 and ldt selector are not saved intentionally */
> tss->eip = ctxt->_eip;
> tss->eflags = ctxt->eflags;
> tss->eax = reg_read(ctxt, VCPU_REGS_RAX);
> @@ -2514,7 +2514,6 @@ static void save_state_to_tss32(struct x86_emulate_ctxt *ctxt,
> tss->ds = get_segment_selector(ctxt, VCPU_SREG_DS);
> tss->fs = get_segment_selector(ctxt, VCPU_SREG_FS);
> tss->gs = get_segment_selector(ctxt, VCPU_SREG_GS);
> - tss->ldt_selector = get_segment_selector(ctxt, VCPU_SREG_LDTR);
> }
>
> static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,

Only this hunk is enough ?

> @@ -2604,6 +2603,8 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
> struct tss_segment_32 tss_seg;
> int ret;
> u32 new_tss_base = get_desc_base(new_desc);
> + u32 eip_offset = offsetof(struct tss_segment_32, eip);
> + u32 ldt_sel_offset = offsetof(struct tss_segment_32, ldt_selector);
>
> ret = ops->read_std(ctxt, old_tss_base, &tss_seg, sizeof tss_seg,
> &ctxt->exception);
> @@ -2613,8 +2614,9 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
>
> save_state_to_tss32(ctxt, &tss_seg);
>
> - ret = ops->write_std(ctxt, old_tss_base, &tss_seg, sizeof tss_seg,
> - &ctxt->exception);
> + /* Only GP registers and segment selectors are saved */
> + ret = ops->write_std(ctxt, old_tss_base + eip_offset, &tss_seg.eip,
> + ldt_sel_offset - eip_offset, &ctxt->exception);
> if (ret != X86EMUL_CONTINUE)
> /* FIXME: need to provide precise fault address */
> return ret;
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-04-28 10:50:06

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH] KVM: x86: Fix CR3 and LDT sel should not be saved in TSS

Il 16/04/2014 21:20, Marcelo Tosatti ha scritto:
> On Mon, Apr 07, 2014 at 06:37:47PM +0300, Nadav Amit wrote:
>> According to Intel specifications, only general purpose registers and segment
>> selectors should are saved in the old TSS during 32-bit task-switch.
>
> should be
>
>> Signed-off-by: Nadav Amit <[email protected]>
>> ---
>> arch/x86/kvm/emulate.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
>> index 205b17e..0dec502 100644
>> --- a/arch/x86/kvm/emulate.c
>> +++ b/arch/x86/kvm/emulate.c
>> @@ -2496,7 +2496,7 @@ static int task_switch_16(struct x86_emulate_ctxt *ctxt,
>> static void save_state_to_tss32(struct x86_emulate_ctxt *ctxt,
>> struct tss_segment_32 *tss)
>> {
>> - tss->cr3 = ctxt->ops->get_cr(ctxt, 3);
>> + /* CR3 and ldt selector are not saved intentionally */
>> tss->eip = ctxt->_eip;
>> tss->eflags = ctxt->eflags;
>> tss->eax = reg_read(ctxt, VCPU_REGS_RAX);
>> @@ -2514,7 +2514,6 @@ static void save_state_to_tss32(struct x86_emulate_ctxt *ctxt,
>> tss->ds = get_segment_selector(ctxt, VCPU_SREG_DS);
>> tss->fs = get_segment_selector(ctxt, VCPU_SREG_FS);
>> tss->gs = get_segment_selector(ctxt, VCPU_SREG_GS);
>> - tss->ldt_selector = get_segment_selector(ctxt, VCPU_SREG_LDTR);
>> }
>>
>> static int load_state_from_tss32(struct x86_emulate_ctxt *ctxt,
>
> Only this hunk is enough ?

I guess there could be a corner case where the beginning or tail of the
TSS is in a read-only page but EIP...LDT is all writable.

Paolo

>> @@ -2604,6 +2603,8 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
>> struct tss_segment_32 tss_seg;
>> int ret;
>> u32 new_tss_base = get_desc_base(new_desc);
>> + u32 eip_offset = offsetof(struct tss_segment_32, eip);
>> + u32 ldt_sel_offset = offsetof(struct tss_segment_32, ldt_selector);
>>
>> ret = ops->read_std(ctxt, old_tss_base, &tss_seg, sizeof tss_seg,
>> &ctxt->exception);
>> @@ -2613,8 +2614,9 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
>>
>> save_state_to_tss32(ctxt, &tss_seg);
>>
>> - ret = ops->write_std(ctxt, old_tss_base, &tss_seg, sizeof tss_seg,
>> - &ctxt->exception);
>> + /* Only GP registers and segment selectors are saved */
>> + ret = ops->write_std(ctxt, old_tss_base + eip_offset, &tss_seg.eip,
>> + ldt_sel_offset - eip_offset, &ctxt->exception);
>> if (ret != X86EMUL_CONTINUE)
>> /* FIXME: need to provide precise fault address */
>> return ret;
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html