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 <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.
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
