[PATCH v5 0/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[PATCH v5 0/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[Wanpeng Li]

There is no easy way to force KVM to run an instruction through the emulator 
(by design as that will expose the x86 emulator as a significant attack-surface).
However, we do wish to expose the x86 emulator in case we are testing it
(e.g. via kvm-unit-tests). Therefore, this patch adds a "force emulation prefix"
that is designed to raise #UD which KVM will trap and it's #UD exit-handler will
match "force emulation prefix" to run instruction after prefix by the x86 emulator.
To not expose the x86 emulator by default, we add a module parameter that should 
be off by default.

A simple testcase here:

#include <stdio.h>
#include <string.h>
   
#define HYPERVISOR_INFO 0x40000000
   
#define CPUID(idx, eax, ebx, ecx, edx) \
    asm volatile ( \
    "ud2a; .ascii \"kvm\"; cpuid" \
    :"=b" (*ebx), "=a" (*eax), "=c" (*ecx), "=d" (*edx) \
        :"0"(idx) );  
   
void main()  
{  
	unsigned int eax, ebx, ecx, edx;  
	char string[13];  
   
	CPUID(HYPERVISOR_INFO, &eax, &ebx, &ecx, &edx);  
	*(unsigned int *)(string + 0) = ebx;  
	*(unsigned int *)(string + 4) = ecx;  
	*(unsigned int *)(string + 8) = edx;  
   
	string[12] = 0;  
	if (strncmp(string, "KVMKVMKVM\0\0\0", 12) == 0)
		printf("kvm guest\n");  
	else  
		printf("bare hardware\n");  
}

v3 -> v4:
 * forwarding emulation failure to userspace
v2 -> v3:
 * fix compile warning
v1 -> v2:
 * update patch descriptions
 * move handle_ud to x86.c, shared by vmx and svm
 * the parameter is in kvm module 
 * rename parameter to force_emulation_prefix

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Liran Alon <liran.alon@oracle.com>

Wanpeng Li (2):
  KVM: X86: Introduce handle_ud()
  KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

 arch/x86/kvm/svm.c |  9 +--------
 arch/x86/kvm/vmx.c | 10 ++--------
 arch/x86/kvm/x86.c | 31 +++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.h |  2 ++
 4 files changed, 36 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH v5 1/2] KVM: X86: Introduce handle_ud()

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

Introduce handle_ud() to handle invalid opcode, this function will be
used by later patches.

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/svm.c |  9 +--------
 arch/x86/kvm/vmx.c | 10 ++--------
 arch/x86/kvm/x86.c | 13 +++++++++++++
 arch/x86/kvm/x86.h |  2 ++
 4 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index f66fc2e..e0a3f56 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -2676,14 +2676,7 @@ static int bp_interception(struct vcpu_svm *svm)
 
 static int ud_interception(struct vcpu_svm *svm)
 {
-	int er;
-
-	er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
-	if (er == EMULATE_USER_EXIT)
-		return 0;
-	if (er != EMULATE_DONE)
-		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
-	return 1;
+	return handle_ud(&svm->vcpu);
 }
 
 static int ac_interception(struct vcpu_svm *svm)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index b2f8a70..0f11243 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6436,14 +6436,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
 	if (is_nmi(intr_info))
 		return 1;  /* already handled by vmx_vcpu_run() */
 
-	if (is_invalid_opcode(intr_info)) {
-		er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
-		if (er == EMULATE_USER_EXIT)
-			return 0;
-		if (er != EMULATE_DONE)
-			kvm_queue_exception(vcpu, UD_VECTOR);
-		return 1;
-	}
+	if (is_invalid_opcode(intr_info))
+		return handle_ud(vcpu);
 
 	error_code = 0;
 	if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7d9a444..1eb495e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4840,6 +4840,19 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
 }
 EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
 
+int handle_ud(struct kvm_vcpu *vcpu)
+{
+	enum emulation_result er;
+
+	er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
+	if (er == EMULATE_USER_EXIT)
+		return 0;
+	if (er != EMULATE_DONE)
+		kvm_queue_exception(vcpu, UD_VECTOR);
+	return 1;
+}
+EXPORT_SYMBOL_GPL(handle_ud);
+
 static int vcpu_is_mmio_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
 			    gpa_t gpa, bool write)
 {
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 1e86174..7d35ce6 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -255,6 +255,8 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
 	gva_t addr, void *val, unsigned int bytes,
 	struct x86_exception *exception);
 
+int handle_ud(struct kvm_vcpu *vcpu);
+
 void kvm_vcpu_mtrr_init(struct kvm_vcpu *vcpu);
 u8 kvm_mtrr_get_guest_memory_type(struct kvm_vcpu *vcpu, gfn_t gfn);
 bool kvm_mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data);
-- 
2.7.4

[PATCH v5 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

There is no easy way to force KVM to run an instruction through the emulator 
(by design as that will expose the x86 emulator as a significant attack-surface).
However, we do wish to expose the x86 emulator in case we are testing it
(e.g. via kvm-unit-tests). Therefore, this patch adds a "force emulation prefix"
that is designed to raise #UD which KVM will trap and it's #UD exit-handler will
match "force emulation prefix" to run instruction after prefix by the x86 emulator.
To not expose the x86 emulator by default, we add a module parameter that should 
be off by default.

A simple testcase here:

#include <stdio.h>
#include <string.h>
   
#define HYPERVISOR_INFO 0x40000000
   
#define CPUID(idx, eax, ebx, ecx, edx) \
    asm volatile (\
    "ud2a; .ascii \"kvm\"; cpuid" \
    :"=b" (*ebx), "=a" (*eax), "=c" (*ecx), "=d" (*edx) \
        :"0"(idx) );  
   
void main()  
{  
	unsigned int eax, ebx, ecx, edx;  
	char string[13];  
   
	CPUID(HYPERVISOR_INFO, &eax, &ebx, &ecx, &edx);  
	*(unsigned int *)(string + 0) = ebx;  
	*(unsigned int *)(string + 4) = ecx;  
	*(unsigned int *)(string + 8) = edx;  
   
	string[12] = 0;  
	if (strncmp(string, "KVMKVMKVM\0\0\0", 12) == 0)
		printf("kvm guest\n");  
	else  
		printf("bare hardware\n");  
}

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/x86.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1eb495e..a55ecef 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
 module_param(enable_vmware_backdoor, bool, S_IRUGO);
 EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
 
+static bool __read_mostly force_emulation_prefix = false;
+module_param(force_emulation_prefix, bool, S_IRUGO);
+
 #define KVM_NR_SHARED_MSRS 16
 
 struct kvm_shared_msrs_global {
@@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
 {
 	enum emulation_result er;
 
+	if (force_emulation_prefix) {
+		char sig[5]; /* ud2; .ascii "kvm" */
+		struct x86_exception e;
+
+		if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
+				kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
+			goto emulate_ud;
+
+		if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
+			kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
+			return emulate_instruction(vcpu, 0) == EMULATE_DONE;
+		}
+	}
+
+emulate_ud:
 	er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
 	if (er == EMULATE_USER_EXIT)
 		return 0;
-- 
2.7.4

Re: [PATCH v5 1/2] KVM: X86: Introduce handle_ud()

[David Hildenbrand]

On 04.04.2018 01:28, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Introduce handle_ud() to handle invalid opcode, this function will be
> used by later patches.
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Liran Alon <liran.alon@oracle.com>
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/svm.c |  9 +--------
>  arch/x86/kvm/vmx.c | 10 ++--------
>  arch/x86/kvm/x86.c | 13 +++++++++++++
>  arch/x86/kvm/x86.h |  2 ++
>  4 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index f66fc2e..e0a3f56 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -2676,14 +2676,7 @@ static int bp_interception(struct vcpu_svm *svm)
>  
>  static int ud_interception(struct vcpu_svm *svm)
>  {
> -	int er;
> -
> -	er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
> -	if (er == EMULATE_USER_EXIT)
> -		return 0;
> -	if (er != EMULATE_DONE)
> -		kvm_queue_exception(&svm->vcpu, UD_VECTOR);
> -	return 1;
> +	return handle_ud(&svm->vcpu);
>  }
>  
>  static int ac_interception(struct vcpu_svm *svm)
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index b2f8a70..0f11243 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -6436,14 +6436,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>  	if (is_nmi(intr_info))
>  		return 1;  /* already handled by vmx_vcpu_run() */
>  
> -	if (is_invalid_opcode(intr_info)) {
> -		er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> -		if (er == EMULATE_USER_EXIT)
> -			return 0;
> -		if (er != EMULATE_DONE)
> -			kvm_queue_exception(vcpu, UD_VECTOR);
> -		return 1;
> -	}
> +	if (is_invalid_opcode(intr_info))
> +		return handle_ud(vcpu);

(maybe different on this branch) isn't "er" now unused?

>  
>  	error_code = 0;
>  	if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7d9a444..1eb495e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4840,6 +4840,19 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
>  }
>  EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
>  
> +int handle_ud(struct kvm_vcpu *vcpu)
> +{
> +	enum emulation_result er;
> +
> +	er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> +	if (er == EMULATE_USER_EXIT)
> +		return 0;
> +	if (er != EMULATE_DONE)
> +		kvm_queue_exception(vcpu, UD_VECTOR);
> +	return 1;

I would now actually prefer

if (er == EMULATE_DONE)
	return 1 ...


Anyhow,

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David / dhildenb

Re: [PATCH v5 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[David Hildenbrand]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1eb495e..a55ecef 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
>  module_param(enable_vmware_backdoor, bool, S_IRUGO);
>  EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>  
> +static bool __read_mostly force_emulation_prefix = false;
> +module_param(force_emulation_prefix, bool, S_IRUGO);
> +
>  #define KVM_NR_SHARED_MSRS 16
>  
>  struct kvm_shared_msrs_global {
> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
>  {
>  	enum emulation_result er;
>  
> +	if (force_emulation_prefix) {
> +		char sig[5]; /* ud2; .ascii "kvm" */
> +		struct x86_exception e;
> +
> +		if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
> +				kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
> +			goto emulate_ud;
> +
> +		if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
> +			kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
> +			return emulate_instruction(vcpu, 0) == EMULATE_DONE;

What if we would have an invalid instruction here? Shouldn't you handle
the emulate_instruction() like below?
(e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
and reuse emulate_ud below)

> +		}
> +	}
> +
> +emulate_ud:
>  	er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>  	if (er == EMULATE_USER_EXIT)
>  		return 0;
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v5 1/2] KVM: X86: Introduce handle_ud()

[Wanpeng Li]

2018-04-04 19:54 GMT+08:00 David Hildenbrand <david@redhat.com>:
> On 04.04.2018 01:28, Wanpeng Li wrote:
>> From: Wanpeng Li <wanpengli@tencent.com>
>>
>> Introduce handle_ud() to handle invalid opcode, this function will be
>> used by later patches.
>>
>> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: Liran Alon <liran.alon@oracle.com>
>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>> ---
>>  arch/x86/kvm/svm.c |  9 +--------
>>  arch/x86/kvm/vmx.c | 10 ++--------
>>  arch/x86/kvm/x86.c | 13 +++++++++++++
>>  arch/x86/kvm/x86.h |  2 ++
>>  4 files changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
>> index f66fc2e..e0a3f56 100644
>> --- a/arch/x86/kvm/svm.c
>> +++ b/arch/x86/kvm/svm.c
>> @@ -2676,14 +2676,7 @@ static int bp_interception(struct vcpu_svm *svm)
>>
>>  static int ud_interception(struct vcpu_svm *svm)
>>  {
>> -     int er;
>> -
>> -     er = emulate_instruction(&svm->vcpu, EMULTYPE_TRAP_UD);
>> -     if (er == EMULATE_USER_EXIT)
>> -             return 0;
>> -     if (er != EMULATE_DONE)
>> -             kvm_queue_exception(&svm->vcpu, UD_VECTOR);
>> -     return 1;
>> +     return handle_ud(&svm->vcpu);
>>  }
>>
>>  static int ac_interception(struct vcpu_svm *svm)
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index b2f8a70..0f11243 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -6436,14 +6436,8 @@ static int handle_exception(struct kvm_vcpu *vcpu)
>>       if (is_nmi(intr_info))
>>               return 1;  /* already handled by vmx_vcpu_run() */
>>
>> -     if (is_invalid_opcode(intr_info)) {
>> -             er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>> -             if (er == EMULATE_USER_EXIT)
>> -                     return 0;
>> -             if (er != EMULATE_DONE)
>> -                     kvm_queue_exception(vcpu, UD_VECTOR);
>> -             return 1;
>> -     }
>> +     if (is_invalid_opcode(intr_info))
>> +             return handle_ud(vcpu);
>
> (maybe different on this branch) isn't "er" now unused?

Hmm, It is used in other place of the function handle_exception.

>
>>
>>       error_code = 0;
>>       if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 7d9a444..1eb495e 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -4840,6 +4840,19 @@ int kvm_write_guest_virt_system(struct x86_emulate_ctxt *ctxt,
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
>>
>> +int handle_ud(struct kvm_vcpu *vcpu)
>> +{
>> +     enum emulation_result er;
>> +
>> +     er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>> +     if (er == EMULATE_USER_EXIT)
>> +             return 0;
>> +     if (er != EMULATE_DONE)
>> +             kvm_queue_exception(vcpu, UD_VECTOR);
>> +     return 1;
>
> I would now actually prefer
>
> if (er == EMULATE_DONE)
>         return 1 ...

Keep the original one I think.

Regards,
Wanpeng Li

Re: [PATCH v5 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[Wanpeng Li]

2018-04-04 19:59 GMT+08:00 David Hildenbrand <david@redhat.com>:
>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 1eb495e..a55ecef 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
>>  module_param(enable_vmware_backdoor, bool, S_IRUGO);
>>  EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>>
>> +static bool __read_mostly force_emulation_prefix = false;
>> +module_param(force_emulation_prefix, bool, S_IRUGO);
>> +
>>  #define KVM_NR_SHARED_MSRS 16
>>
>>  struct kvm_shared_msrs_global {
>> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
>>  {
>>       enum emulation_result er;
>>
>> +     if (force_emulation_prefix) {
>> +             char sig[5]; /* ud2; .ascii "kvm" */
>> +             struct x86_exception e;
>> +
>> +             if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
>> +                             kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
>> +                     goto emulate_ud;
>> +
>> +             if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
>> +                     kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
>> +                     return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>
> What if we would have an invalid instruction here? Shouldn't you handle
> the emulate_instruction() like below?
> (e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
> and reuse emulate_ud below)

emulate_instruction(vcpu, 0) can handle invalid instruction.

Regards,
Wanpeng Li

Re: [PATCH v5 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[Paolo Bonzini]

On 04/04/2018 15:35, Wanpeng Li wrote:
> 2018-04-04 19:59 GMT+08:00 David Hildenbrand <david@redhat.com>:
>>
>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>> index 1eb495e..a55ecef 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
>>>  module_param(enable_vmware_backdoor, bool, S_IRUGO);
>>>  EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>>>
>>> +static bool __read_mostly force_emulation_prefix = false;
>>> +module_param(force_emulation_prefix, bool, S_IRUGO);
>>> +
>>>  #define KVM_NR_SHARED_MSRS 16
>>>
>>>  struct kvm_shared_msrs_global {
>>> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
>>>  {
>>>       enum emulation_result er;
>>>
>>> +     if (force_emulation_prefix) {
>>> +             char sig[5]; /* ud2; .ascii "kvm" */
>>> +             struct x86_exception e;
>>> +
>>> +             if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
>>> +                             kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
>>> +                     goto emulate_ud;
>>> +
>>> +             if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
>>> +                     kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
>>> +                     return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>>
>> What if we would have an invalid instruction here? Shouldn't you handle
>> the emulate_instruction() like below?
>> (e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
>> and reuse emulate_ud below)
> 
> emulate_instruction(vcpu, 0) can handle invalid instruction.

But David's observation is still better because your code doesn't handle
usermode exits.  I've fixed this up.

Paolo

Re: [PATCH v5 1/2] KVM: X86: Introduce handle_ud()

[Paolo Bonzini]

On 04/04/2018 13:54, David Hildenbrand wrote:
>> +{
>> +	enum emulation_result er;
>> +
>> +	er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>> +	if (er == EMULATE_USER_EXIT)
>> +		return 0;
>> +	if (er != EMULATE_DONE)
>> +		kvm_queue_exception(vcpu, UD_VECTOR);
>> +	return 1;
> I would now actually prefer
> 
> if (er == EMULATE_DONE)
> 	return 1 ...

Why?  The return statement would be duplicated.

Paolo

Re: [PATCH v5 1/2] KVM: X86: Introduce handle_ud()

[David Hildenbrand]

On 04.04.2018 19:12, Paolo Bonzini wrote:
> On 04/04/2018 13:54, David Hildenbrand wrote:
>>> +{
>>> +	enum emulation_result er;
>>> +
>>> +	er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
>>> +	if (er == EMULATE_USER_EXIT)
>>> +		return 0;
>>> +	if (er != EMULATE_DONE)
>>> +		kvm_queue_exception(vcpu, UD_VECTOR);
>>> +	return 1;
>> I would now actually prefer
>>
>> if (er == EMULATE_DONE)
>> 	return 1 ...
> 
> Why?  The return statement would be duplicated.
> 
> Paolo
> 

I was talking about two equality checks vs. 1 equality and 1 inequality
check.

er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
if (er == EMULATE_USER_EXIT)
	return 0;
else if (er == EMULATE_DONE)
	return 1;
return kvm_queue_exception(vcpu, UD_VECTOR);


-- 

Thanks,

David / dhildenb

Re: [PATCH v5 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[Wanpeng Li]

2018-04-05 1:09 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
> On 04/04/2018 15:35, Wanpeng Li wrote:
>> 2018-04-04 19:59 GMT+08:00 David Hildenbrand <david@redhat.com>:
>>>
>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>>> index 1eb495e..a55ecef 100644
>>>> --- a/arch/x86/kvm/x86.c
>>>> +++ b/arch/x86/kvm/x86.c
>>>> @@ -146,6 +146,9 @@ bool __read_mostly enable_vmware_backdoor = false;
>>>>  module_param(enable_vmware_backdoor, bool, S_IRUGO);
>>>>  EXPORT_SYMBOL_GPL(enable_vmware_backdoor);
>>>>
>>>> +static bool __read_mostly force_emulation_prefix = false;
>>>> +module_param(force_emulation_prefix, bool, S_IRUGO);
>>>> +
>>>>  #define KVM_NR_SHARED_MSRS 16
>>>>
>>>>  struct kvm_shared_msrs_global {
>>>> @@ -4844,6 +4847,21 @@ int handle_ud(struct kvm_vcpu *vcpu)
>>>>  {
>>>>       enum emulation_result er;
>>>>
>>>> +     if (force_emulation_prefix) {
>>>> +             char sig[5]; /* ud2; .ascii "kvm" */
>>>> +             struct x86_exception e;
>>>> +
>>>> +             if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
>>>> +                             kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e))
>>>> +                     goto emulate_ud;
>>>> +
>>>> +             if (memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
>>>> +                     kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
>>>> +                     return emulate_instruction(vcpu, 0) == EMULATE_DONE;
>>>
>>> What if we would have an invalid instruction here? Shouldn't you handle
>>> the emulate_instruction() like below?
>>> (e.g. keep a variable with the emulation type (0 vs EMULTYPE_TRAP_UD)
>>> and reuse emulate_ud below)
>>
>> emulate_instruction(vcpu, 0) can handle invalid instruction.
>
> But David's observation is still better because your code doesn't handle usermode exits.

My code handles it, return emulate_instruction(vcpu, 0) ==
EMULATE_DONE, it will return 0 since EMULATE_USER_EXIT == EMULATE_DONE
fails.

> I've fixed this up.

Thanks. The codes similar to my v3 but more beauty. :) I change to
this view since Radim's comments to v3
https://www.spinics.net/lists/kvm/msg166999.html

Regards,
Wanpeng Li

Re: [PATCH v5 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

[Paolo Bonzini]

On 05/04/2018 02:04, Wanpeng Li wrote:
>>> emulate_instruction(vcpu, 0) can handle invalid instruction.
>> But David's observation is still better because your code doesn't handle usermode exits.
> My code handles it, return emulate_instruction(vcpu, 0) ==
> EMULATE_DONE, it will return 0 since EMULATE_USER_EXIT == EMULATE_DONE
> fails.
> 
>> I've fixed this up.
> Thanks. The codes similar to my v3 but more beauty. :) I change to
> this view since Radim's comments to v3
> https://www.spinics.net/lists/kvm/msg166999.html

And after I actually woke up I think I disagree with Radim.  Tests can
trap the #UD to test emulation at CPL0 and skip or fail the test for
instructions unknown to the emulator.  It's much better than sending an
emulation failure to userspace, which would abort the guest.

Paolo