Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks

[Kevin Wolf <kwolf@redhat.com>]

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