linux-kernel - Re: [PATCH v3 2/7] x86: kvm: svm: propagate errors from skip_emulated

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Aug 2019 11:31:47 -0700
From:   Sean Christopherson <sean.j.christopherson@...el.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Joerg Roedel <joro@...tes.org>,
        Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH v3 2/7] x86: kvm: svm: propagate errors from
 skip_emulated_instruction()

On Thu, Aug 08, 2019 at 07:30:46PM +0200, Vitaly Kuznetsov wrote:
> On AMD, kvm_x86_ops->skip_emulated_instruction(vcpu) can, in theory,
> fail: in !nrips case we call kvm_emulate_instruction(EMULTYPE_SKIP).
> Currently, we only do printk(KERN_DEBUG) when this happens and this
> is not ideal. Propagate the error up the stack.
> 
> On VMX, skip_emulated_instruction() doesn't fail, we have two call
> sites calling it explicitly: handle_exception_nmi() and
> handle_task_switch(), we can just ignore the result.
> 
> On SVM, we also have two explicit call sites:
> svm_queue_exception() and it seems we don't need to do anything there as
> we check if RIP was advanced or not. In task_switch_interception(),
> however, we are better off not proceeding to kvm_task_switch() in case
> skip_emulated_instruction() failed.
> 
> Suggested-by: Sean Christopherson <sean.j.christopherson@...el.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---

...

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 074385c86c09..2579e7a6d59d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1473,7 +1473,7 @@ static int vmx_rtit_ctl_check(struct kvm_vcpu *vcpu, u64 data)
>  }
>  
>  
> -static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
> +static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  {
>  	unsigned long rip;
>  
> @@ -1483,6 +1483,8 @@ static void skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  
>  	/* skipping an emulated instruction also counts */
>  	vmx_set_interrupt_shadow(vcpu, 0);
> +
> +	return EMULATE_DONE;
>  }
>  
>  static void vmx_clear_hlt(struct kvm_vcpu *vcpu)
> @@ -4547,7 +4549,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>  			vcpu->arch.dr6 &= ~DR_TRAP_BITS;
>  			vcpu->arch.dr6 |= dr6 | DR6_RTM;
>  			if (is_icebp(intr_info))
> -				skip_emulated_instruction(vcpu);
> +				(void)skip_emulated_instruction(vcpu);
>  
>  			kvm_queue_exception(vcpu, DB_VECTOR);
>  			return 1;
> @@ -5057,7 +5059,7 @@ static int handle_task_switch(struct kvm_vcpu *vcpu)
>  	if (!idt_v || (type != INTR_TYPE_HARD_EXCEPTION &&
>  		       type != INTR_TYPE_EXT_INTR &&
>  		       type != INTR_TYPE_NMI_INTR))
> -		skip_emulated_instruction(vcpu);
> +		(void)skip_emulated_instruction(vcpu);

Maybe a silly idea, but what if we squash the return value in a dedicated
helper, with a big "DO NOT USE" comment above the int-returning function, e.g.:

static int __skip_emulated_instruction(struct kvm_vcpu *vcpu)
{
	unsigned long rip;

	rip = kvm_rip_read(vcpu);
	rip += vmcs_read32(VM_EXIT_INSTRUCTION_LEN);
	kvm_rip_write(vcpu, rip);

	/* skipping an emulated instruction also counts */
	vmx_set_interrupt_shadow(vcpu, 0);

	return EMULATE_DONE;
}

static inline void skip_emulated_instruction(struct kvm_vcpu *vcpu)
{
	(void)__skip_emulated_instruction(vcpu);
}


Alternatively, the inner function could be void, but on my system that
adds an extra call in the wrapper, i.e. in the kvm_skip_emulated...()
path.  The above approach generates the same code as your patch, e.g.
allows the compiler to decide whether or not to inline the meat of the
code.

>  	if (kvm_task_switch(vcpu, tss_selector,
>  			    type == INTR_TYPE_SOFT_INTR ? idt_index : -1, reason,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c6d951cbd76c..a97818b1111d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6383,9 +6383,11 @@ static void kvm_vcpu_do_singlestep(struct kvm_vcpu *vcpu, int *r)
>  int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  {
>  	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> -	int r = EMULATE_DONE;
> +	int r;
>  
> -	kvm_x86_ops->skip_emulated_instruction(vcpu);
> +	r = kvm_x86_ops->skip_emulated_instruction(vcpu);
> +	if (r != EMULATE_DONE)

This should probably be wrapped with unlikely.

> +		return 0;
>  
>  	/*
>  	 * rflags is the old, "raw" value of the flags.  The new value has
> -- 
> 2.20.1
>