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

[Gleb Natapov <gleb@redhat.com>]

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

>  
> +	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.