2018-03-30 09:09:12

by Wanpeng Li

[permalink] [raw]
Subject: [PATCH v4 0/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

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 <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Liran Alon <[email protected]>

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 | 28 ++++++++++++++++++++++++++++
arch/x86/kvm/x86.h | 2 ++
4 files changed, 33 insertions(+), 16 deletions(-)

--
2.7.4



2018-03-30 09:09:27

by Wanpeng Li

[permalink] [raw]
Subject: [PATCH v4 1/2] KVM: X86: Introduce handle_ud()

From: Wanpeng Li <[email protected]>

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

Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>
Reviewed-By: Liran Alon <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Liran Alon <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
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


2018-03-30 09:11:30

by Wanpeng Li

[permalink] [raw]
Subject: [PATCH v4 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

From: Wanpeng Li <[email protected]>

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 <[email protected]>
Reviewed-By: Liran Alon <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Andrew Cooper <[email protected]>
Cc: Konrad Rzeszutek Wilk <[email protected]>
Cc: Liran Alon <[email protected]>
Signed-off-by: Wanpeng Li <[email protected]>
---
arch/x86/kvm/x86.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1eb495e..c619564 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,18 @@ 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;
+
+ kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
+ kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e);
+ 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;
+ }
+ }
+
er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
if (er == EMULATE_USER_EXIT)
return 0;
--
2.7.4


2018-04-03 19:25:41

by Radim Krčmář

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

2018-03-30 02:06-0700, Wanpeng Li:
> From: Wanpeng Li <[email protected]>
>
> 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 <[email protected]>
> Reviewed-By: Liran Alon <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Andrew Cooper <[email protected]>
> Cc: Konrad Rzeszutek Wilk <[email protected]>
> Cc: Liran Alon <[email protected]>
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> arch/x86/kvm/x86.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1eb495e..c619564 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,18 @@ 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;
> +
> + kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
> + kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e);

Looking at it again, we should skip the following check if the call
failed (the sig is undefined in that case).

With that, or even without as we're talking about a feature that has no
place in any production system,

Reviewed-by: Radim Krčmář <[email protected]>

> + 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;
> + }
> + }
> +
> er = emulate_instruction(vcpu, EMULTYPE_TRAP_UD);
> if (er == EMULATE_USER_EXIT)
> return 0;
> --
> 2.7.4
>

2018-04-03 23:33:33

by Wanpeng Li

[permalink] [raw]
Subject: Re: [PATCH v4 2/2] KVM: X86: Add Force Emulation Prefix for "emulate the next instruction"

2018-04-04 3:24 GMT+08:00 Radim Krčmář <[email protected]>:
> 2018-03-30 02:06-0700, Wanpeng Li:
>> From: Wanpeng Li <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-By: Liran Alon <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Radim Krčmář <[email protected]>
>> Cc: Andrew Cooper <[email protected]>
>> Cc: Konrad Rzeszutek Wilk <[email protected]>
>> Cc: Liran Alon <[email protected]>
>> Signed-off-by: Wanpeng Li <[email protected]>
>> ---
>> arch/x86/kvm/x86.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 1eb495e..c619564 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,18 @@ 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;
>> +
>> + kvm_read_guest_virt(&vcpu->arch.emulate_ctxt,
>> + kvm_get_linear_rip(vcpu), sig, sizeof(sig), &e);
>
> Looking at it again, we should skip the following check if the call
> failed (the sig is undefined in that case).

Ok, will fallback to normal emulate_ud when this fail.

>
> With that, or even without as we're talking about a feature that has no
> place in any production system,
>
> Reviewed-by: Radim Krčmář <[email protected]>

Thanks Radim. :)

Regards,
Wanpeng Li