On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
> Currently, all task switches check privileges against the DPL of the
> TSS. This is only correct for jmp/call to a TSS. If a task gate is used,
> the DPL of this take gate is used for the check instead. Exceptions,
> external interrupts and iret shouldn't perform any check.
>
> This patch fixes the problem for VMX. For SVM, the logic used to
> determine the source of the task switch is buggy, so we can't pass
> useful information to the emulator there and just disable the check in
> all cases.
>
Is this accurate description? Since on SVM you never get
TASK_SWITCH_GATE reason the check is always done using TSS dpl, not
disabled in all cases, correct?
> Signed-off-by: Kevin Wolf <kwolf@xxxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_emulate.h | 2 +-
> arch/x86/include/asm/kvm_host.h | 4 +-
> arch/x86/kvm/emulate.c | 51 +++++++++++++++++++++++++++++++-----
> arch/x86/kvm/svm.c | 3 +-
> arch/x86/kvm/vmx.c | 5 ++-
> arch/x86/kvm/x86.c | 6 ++--
> 6 files changed, 55 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index ab4092e..c8a9cf3 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -372,7 +372,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt);
> #define EMULATION_INTERCEPTED 2
> int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
> int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
> - u16 tss_selector, int reason,
> + u16 tss_selector, int idt_index, int reason,
> bool has_error_code, u32 error_code);
> int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
> #endif /* _ASM_X86_KVM_X86_EMULATE_H */
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 52d6640..0533fc4 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -741,8 +741,8 @@ int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
> void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
> int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector, int seg);
>
> -int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
> - bool has_error_code, u32 error_code);
> +int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
> + int reason, bool has_error_code, u32 error_code);
>
> int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
> int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 05a562b..1b98a2c 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -1151,6 +1151,22 @@ static int pio_in_emulated(struct x86_emulate_ctxt *ctxt,
> return 1;
> }
>
> +static int read_interrupt_descriptor(struct x86_emulate_ctxt *ctxt,
> + u16 index, struct kvm_desc_struct *desc)
> +{
> + struct kvm_desc_ptr dt;
> + ulong addr;
> +
> + ctxt->ops->get_idt(ctxt, &dt);
> +
> + if (dt.size < index * 8 + 7)
> + return emulate_gp(ctxt, index << 3 | 0x2);
> +
> + addr = dt.address + index * 8;
> + return ctxt->ops->read_std(ctxt, addr, desc, sizeof *desc,
> + &ctxt->exception);
> +}
> +
> static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
> u16 selector, struct desc_ptr *dt)
> {
> @@ -2350,7 +2366,7 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
> }
>
> static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
> - u16 tss_selector, int reason,
> + u16 tss_selector, int idt_index, int reason,
> bool has_error_code, u32 error_code)
> {
> struct x86_emulate_ops *ops = ctxt->ops;
> @@ -2360,6 +2376,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
> ulong old_tss_base =
> ops->get_cached_segment_base(ctxt, VCPU_SREG_TR);
> u32 desc_limit;
> + int dpl;
>
> /* FIXME: old_tss_base == ~0 ? */
>
> @@ -2372,12 +2389,32 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
>
> /* FIXME: check that next_tss_desc is tss */
>
> - if (reason != TASK_SWITCH_IRET) {
> - if ((tss_selector & 3) > next_tss_desc.dpl ||
> - ops->cpl(ctxt) > next_tss_desc.dpl)
> - return emulate_gp(ctxt, 0);
> + /*
> + * Check privileges. The three cases are task switch caused by...
> + *
> + * 1. Software interrupt: Check against DPL of the task gate
> + * 2. Exception/IRQ/iret: No check is performed
> + * 3. jmp/call: Check agains DPL of the TSS
> + */
> + dpl = -1;
> + if (reason == TASK_SWITCH_GATE) {
> + if (idt_index != -1) {
> + struct kvm_desc_struct task_gate_desc;
> +
> + ret = read_interrupt_descriptor(ctxt, idt_index,
> + &task_gate_desc);
> + if (ret != X86EMUL_CONTINUE)
> + return ret;
> +
> + dpl = task_gate_desc.dpl;
> + }
> + } else if (reason != TASK_SWITCH_IRET) {
> + dpl = next_tss_desc.dpl;
> }
No need parentheses around one statement.
>
> + if (dpl != -1 && ((tss_selector & 3) > dpl || ops->cpl(ctxt) > dpl))
> + return emulate_gp(ctxt, 0);
> +
> desc_limit = desc_limit_scaled(&next_tss_desc);
> if (!next_tss_desc.p ||
> ((desc_limit < 0x67 && (next_tss_desc.type & 8)) ||
> @@ -2430,7 +2467,7 @@ static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
> }
>
> int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
> - u16 tss_selector, int reason,
> + u16 tss_selector, int idt_index, int reason,
> bool has_error_code, u32 error_code)
> {
> int rc;
> @@ -2438,7 +2475,7 @@ int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
> ctxt->_eip = ctxt->eip;
> ctxt->dst.type = OP_NONE;
>
> - rc = emulator_do_task_switch(ctxt, tss_selector, reason,
> + rc = emulator_do_task_switch(ctxt, tss_selector, idt_index, reason,
> has_error_code, error_code);
>
> if (rc == X86EMUL_CONTINUE)
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 5fa553b..b2223dc 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2730,7 +2730,8 @@ static int task_switch_interception(struct vcpu_svm *svm)
> (int_vec == OF_VECTOR || int_vec == BP_VECTOR)))
> skip_emulated_instruction(&svm->vcpu);
>
> - if (kvm_task_switch(&svm->vcpu, tss_selector, reason,
> + /* TODO Pass IDT vector for software interrupts */
> + if (kvm_task_switch(&svm->vcpu, tss_selector, -1, reason,
> has_error_code, error_code) == EMULATE_FAIL) {
> svm->vcpu.run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> svm->vcpu.run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 906a7e8..23fb328 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4712,8 +4712,9 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
> type != INTR_TYPE_NMI_INTR))
> skip_emulated_instruction(vcpu);
>
> - if (kvm_task_switch(vcpu, tss_selector, reason,
> - has_error_code, error_code) == EMULATE_FAIL) {
> + if (kvm_task_switch(vcpu, tss_selector,
> + type == INTR_TYPE_SOFT_INTR ? idt_v : -1, reason,
> + has_error_code, error_code) == EMULATE_FAIL) {
> vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION;
> vcpu->run->internal.ndata = 0;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1171def..dc3e945 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5541,15 +5541,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
> - bool has_error_code, u32 error_code)
> +int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
> + int reason, bool has_error_code, u32 error_code)
> {
> struct x86_emulate_ctxt *ctxt = &vcpu->arch.emulate_ctxt;
> int ret;
>
> init_emulate_ctxt(vcpu);
>
> - ret = emulator_task_switch(ctxt, tss_selector, reason,
> + ret = emulator_task_switch(ctxt, tss_selector, idt_index, reason,
> has_error_code, error_code);
>
> if (ret)
> --
> 1.7.6.5
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
