[RESEND,07/13] KVM: x86: Add explicit flag for forced emulation on #UD
diff mbox series

Message ID 20190823010709.24879-8-sean.j.christopherson@intel.com
State New
Headers show
Series
  • KVM: x86: Remove emulation_result enums
Related show

Commit Message

Sean Christopherson Aug. 23, 2019, 1:07 a.m. UTC
Add an explicit emulation type for forced #UD emulation and use it to
detect that KVM should unconditionally inject a #UD instead of falling
into its standard emulation failure handling.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/x86.c              | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Liran Alon Aug. 23, 2019, 1:47 p.m. UTC | #1
> On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> Add an explicit emulation type for forced #UD emulation and use it to
> detect that KVM should unconditionally inject a #UD instead of falling
> into its standard emulation failure handling.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>

The name "forced emulation on #UD" is not clear to me.

If I understand correctly, EMULTYPE_TRAP_UD is currently used to indicate
that in case the x86 emulator fails to decode instruction, the caller would like
the x86 emulator to fail early such that it can handle this condition properly.
Thus, I would rename it EMULTYPE_TRAP_DECODE_FAILURE.

But this new flag seems to do the same. So I’m left confused.
I’m probably missing something trivial here.

-Liran

> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/x86.c              | 5 +++--
> 2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index d1d5b5ca1195..a38c93362945 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1318,6 +1318,7 @@ enum emulation_result {
> #define EMULTYPE_TRAP_UD	    (1 << 1)
> #define EMULTYPE_SKIP		    (1 << 2)
> #define EMULTYPE_ALLOW_RETRY	    (1 << 3)
> +#define EMULTYPE_TRAP_UD_FORCED	    (1 << 4)
> #define EMULTYPE_VMWARE_GP	    (1 << 5)
> int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
> int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 228ca71d5b01..a1f9e36b2d58 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5337,7 +5337,7 @@ int handle_ud(struct kvm_vcpu *vcpu)
> 				sig, sizeof(sig), &e) == 0 &&
> 	    memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
> 		kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
> -		emul_type = 0;
> +		emul_type = EMULTYPE_TRAP_UD_FORCED;
> 	}
> 
> 	er = kvm_emulate_instruction(vcpu, emul_type);
> @@ -6532,7 +6532,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
> 		trace_kvm_emulate_insn_start(vcpu);
> 		++vcpu->stat.insn_emulation;
> 		if (r != EMULATION_OK)  {
> -			if (emulation_type & EMULTYPE_TRAP_UD)
> +			if ((emulation_type & EMULTYPE_TRAP_UD) ||
> +			    (emulation_type & EMULTYPE_TRAP_UD_FORCED))
> 				return EMULATE_FAIL;
> 			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
> 						emulation_type))
> -- 
> 2.22.0
>
Sean Christopherson Aug. 23, 2019, 2:44 p.m. UTC | #2
On Fri, Aug 23, 2019 at 04:47:14PM +0300, Liran Alon wrote:
> 
> 
> > On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > Add an explicit emulation type for forced #UD emulation and use it to
> > detect that KVM should unconditionally inject a #UD instead of falling
> > into its standard emulation failure handling.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> The name "forced emulation on #UD" is not clear to me.
> 
> If I understand correctly, EMULTYPE_TRAP_UD is currently used to indicate
> that in case the x86 emulator fails to decode instruction, the caller would
> like the x86 emulator to fail early such that it can handle this condition
> properly.  Thus, I would rename it EMULTYPE_TRAP_DECODE_FAILURE.

EMULTYPE_TRAP_UD is used when KVM intercepts a #UD from hardware.  KVM
only emulates select instructions in this case in order to minmize the
emulator attack surface, e.g.:

	if (unlikely(ctxt->ud) && likely(!(ctxt->d & EmulateOnUD)))
		return EMULATION_FAILED;

To enable testing of the emulator, KVM recognizes a special "opcode" that
triggers full emulation on #UD, e.g. ctxt->ud is false when the #UD was
triggered with the magic prefix.  The prefix is only recognized when the
module param force_emulation_prefix is toggled on, hence the name
EMULTYPE_TRAP_UD_FORCED.

> But this new flag seems to do the same. So I’m left confused.  I’m probably
> missing something trivial here.
Liran Alon Aug. 23, 2019, 3:31 p.m. UTC | #3
> On 23 Aug 2019, at 17:44, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Fri, Aug 23, 2019 at 04:47:14PM +0300, Liran Alon wrote:
>> 
>> 
>>> On 23 Aug 2019, at 4:07, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>> 
>>> Add an explicit emulation type for forced #UD emulation and use it to
>>> detect that KVM should unconditionally inject a #UD instead of falling
>>> into its standard emulation failure handling.
>>> 
>>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> 
>> The name "forced emulation on #UD" is not clear to me.
>> 
>> If I understand correctly, EMULTYPE_TRAP_UD is currently used to indicate
>> that in case the x86 emulator fails to decode instruction, the caller would
>> like the x86 emulator to fail early such that it can handle this condition
>> properly.  Thus, I would rename it EMULTYPE_TRAP_DECODE_FAILURE.
> 
> EMULTYPE_TRAP_UD is used when KVM intercepts a #UD from hardware.  KVM
> only emulates select instructions in this case in order to minmize the
> emulator attack surface, e.g.:
> 
> 	if (unlikely(ctxt->ud) && likely(!(ctxt->d & EmulateOnUD)))
> 		return EMULATION_FAILED;
> 
> To enable testing of the emulator, KVM recognizes a special "opcode" that
> triggers full emulation on #UD, e.g. ctxt->ud is false when the #UD was
> triggered with the magic prefix.  The prefix is only recognized when the
> module param force_emulation_prefix is toggled on, hence the name
> EMULTYPE_TRAP_UD_FORCED.

Ah-ha. This makes sense. Thanks for the explanation.
I would say it’s worth putting a comment in it in code…

> 
>> But this new flag seems to do the same. So I’m left confused.  I’m probably
>> missing something trivial here.

Patch
diff mbox series

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d1d5b5ca1195..a38c93362945 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1318,6 +1318,7 @@  enum emulation_result {
 #define EMULTYPE_TRAP_UD	    (1 << 1)
 #define EMULTYPE_SKIP		    (1 << 2)
 #define EMULTYPE_ALLOW_RETRY	    (1 << 3)
+#define EMULTYPE_TRAP_UD_FORCED	    (1 << 4)
 #define EMULTYPE_VMWARE_GP	    (1 << 5)
 int kvm_emulate_instruction(struct kvm_vcpu *vcpu, int emulation_type);
 int kvm_emulate_instruction_from_buffer(struct kvm_vcpu *vcpu,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 228ca71d5b01..a1f9e36b2d58 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5337,7 +5337,7 @@  int handle_ud(struct kvm_vcpu *vcpu)
 				sig, sizeof(sig), &e) == 0 &&
 	    memcmp(sig, "\xf\xbkvm", sizeof(sig)) == 0) {
 		kvm_rip_write(vcpu, kvm_rip_read(vcpu) + sizeof(sig));
-		emul_type = 0;
+		emul_type = EMULTYPE_TRAP_UD_FORCED;
 	}
 
 	er = kvm_emulate_instruction(vcpu, emul_type);
@@ -6532,7 +6532,8 @@  int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 		trace_kvm_emulate_insn_start(vcpu);
 		++vcpu->stat.insn_emulation;
 		if (r != EMULATION_OK)  {
-			if (emulation_type & EMULTYPE_TRAP_UD)
+			if ((emulation_type & EMULTYPE_TRAP_UD) ||
+			    (emulation_type & EMULTYPE_TRAP_UD_FORCED))
 				return EMULATE_FAIL;
 			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
 						emulation_type))