Am 24.01.2012 10:52, schrieb Gleb Natapov: > 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?
Hm, right, they turn out as TASK_SWITCH_CALL, so we check against the TSS. I'll fix the commit message if I need to do a v2. >> Signed-off-by: Kevin Wolf <kw...@redhat.com> >> --- >> 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. Documentation/CodingStyle says: "This does not apply if only one branch of a conditional statement is a single statement; in the latter case use braces in both branches:" Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html