[PATCH v2 0/3] fix emulation error on Windows bootup

[PATCH v2 0/3] fix emulation error on Windows bootup

[Jan Dakinevich]

This series intended to fix (again) a bug that was a subject of the 
following change:

  6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")

Suddenly, that fix had a couple mistakes. First, ctxt->have_exception was 
not set if fault happened during instruction decoding. Second, returning 
value of inject_emulated_instruction was used to make the decision to 
reenter guest, but this could happen iff on nested page fault, that is not 
the scope where this bug could occur.

v1:
  https://lkml.org/lkml/2019/8/27/881

v2
  - reorder commits, rebase on top kvm/queue
  - add sanity check for exception value of exception vector

Jan Dakinevich (3):
  KVM: x86: always stop emulation on page fault
  KVM: x86: make exception_class() and exception_type() globally visible
  KVM: x86: set ctxt->have_exception in x86_decode_insn()

 arch/x86/kvm/emulate.c |  5 +++++
 arch/x86/kvm/x86.c     | 50 +++-----------------------------------------------
 arch/x86/kvm/x86.h     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 47 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] KVM: x86: always stop emulation on page fault

[Jan Dakinevich]

inject_emulated_exception() returns true if and only if nested page
fault happens. However, page fault can come from guest page tables
walk, either nested or not nested. In both cases we should stop an
attempt to read under RIP and give guest to step over its own page
fault handler.

Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/x86.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b4cfd78..903fb7c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6537,8 +6537,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu,
 			if (reexecute_instruction(vcpu, cr2, write_fault_to_spt,
 						emulation_type))
 				return EMULATE_DONE;
-			if (ctxt->have_exception && inject_emulated_exception(vcpu))
+			if (ctxt->have_exception) {
+				inject_emulated_exception(vcpu);
 				return EMULATE_DONE;
+			}
 			if (emulation_type & EMULTYPE_SKIP)
 				return EMULATE_FAIL;
 			return handle_emulation_failure(vcpu, emulation_type);
-- 
2.1.4

[PATCH v2 2/3] KVM: x86: make exception_class() and exception_type() globally visible

[Jan Dakinevich]

exception_type() function was moved for upcoming sanity check in
emulation code. exceptions_class() function is not supposed to be used
right now, but it was moved as well to keep things together.

Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/x86.c | 46 ----------------------------------------------
 arch/x86/kvm/x86.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 903fb7c..2b69ae0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -364,52 +364,6 @@ asmlinkage __visible void kvm_spurious_fault(void)
 }
 EXPORT_SYMBOL_GPL(kvm_spurious_fault);
 
-#define EXCPT_BENIGN		0
-#define EXCPT_CONTRIBUTORY	1
-#define EXCPT_PF		2
-
-static int exception_class(int vector)
-{
-	switch (vector) {
-	case PF_VECTOR:
-		return EXCPT_PF;
-	case DE_VECTOR:
-	case TS_VECTOR:
-	case NP_VECTOR:
-	case SS_VECTOR:
-	case GP_VECTOR:
-		return EXCPT_CONTRIBUTORY;
-	default:
-		break;
-	}
-	return EXCPT_BENIGN;
-}
-
-#define EXCPT_FAULT		0
-#define EXCPT_TRAP		1
-#define EXCPT_ABORT		2
-#define EXCPT_INTERRUPT		3
-
-static int exception_type(int vector)
-{
-	unsigned int mask;
-
-	if (WARN_ON(vector > 31 || vector == NMI_VECTOR))
-		return EXCPT_INTERRUPT;
-
-	mask = 1 << vector;
-
-	/* #DB is trap, as instruction watchpoints are handled elsewhere */
-	if (mask & ((1 << DB_VECTOR) | (1 << BP_VECTOR) | (1 << OF_VECTOR)))
-		return EXCPT_TRAP;
-
-	if (mask & ((1 << DF_VECTOR) | (1 << MC_VECTOR)))
-		return EXCPT_ABORT;
-
-	/* Reserved exceptions will result in fault */
-	return EXCPT_FAULT;
-}
-
 void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu)
 {
 	unsigned nr = vcpu->arch.exception.nr;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index b5274e2..2b66347 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -369,4 +369,50 @@ static inline bool kvm_pat_valid(u64 data)
 void kvm_load_guest_xcr0(struct kvm_vcpu *vcpu);
 void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu);
 
+#define EXCPT_BENIGN		0
+#define EXCPT_CONTRIBUTORY	1
+#define EXCPT_PF		2
+
+static inline int exception_class(int vector)
+{
+	switch (vector) {
+	case PF_VECTOR:
+		return EXCPT_PF;
+	case DE_VECTOR:
+	case TS_VECTOR:
+	case NP_VECTOR:
+	case SS_VECTOR:
+	case GP_VECTOR:
+		return EXCPT_CONTRIBUTORY;
+	default:
+		break;
+	}
+	return EXCPT_BENIGN;
+}
+
+#define EXCPT_FAULT		0
+#define EXCPT_TRAP		1
+#define EXCPT_ABORT		2
+#define EXCPT_INTERRUPT		3
+
+static inline int exception_type(int vector)
+{
+	unsigned int mask;
+
+	if (WARN_ON(vector > 31 || vector == NMI_VECTOR))
+		return EXCPT_INTERRUPT;
+
+	mask = 1 << vector;
+
+	/* #DB is trap, as instruction watchpoints are handled elsewhere */
+	if (mask & ((1 << DB_VECTOR) | (1 << BP_VECTOR) | (1 << OF_VECTOR)))
+		return EXCPT_TRAP;
+
+	if (mask & ((1 << DF_VECTOR) | (1 << MC_VECTOR)))
+		return EXCPT_ABORT;
+
+	/* Reserved exceptions will result in fault */
+	return EXCPT_FAULT;
+}
+
 #endif
-- 
2.1.4

[PATCH v2 3/3] KVM: x86: set ctxt->have_exception in x86_decode_insn()

[Jan Dakinevich]

x86_emulate_instruction() takes into account ctxt->have_exception flag
during instruction decoding, but in practice this flag is never set in
x86_decode_insn().

Fixes: 6ea6e84 ("KVM: x86: inject exceptions produced by x86_decode_insn")
Cc: Denis Lunev <den@virtuozzo.com>
Cc: Roman Kagan <rkagan@virtuozzo.com>
Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
---
 arch/x86/kvm/emulate.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index bef3c3c..74b4d79 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5416,6 +5416,11 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len)
 					ctxt->memopp->addr.mem.ea + ctxt->_eip);
 
 done:
+	if (rc == X86EMUL_PROPAGATE_FAULT) {
+		WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR ||
+			     exception_type(ctxt->exception.vector) == EXCPT_TRAP);
+		ctxt->have_exception = true;
+	}
 	return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
 }
 
-- 
2.1.4

Re: [PATCH v2 2/3] KVM: x86: make exception_class() and exception_type() globally visible

[Sean Christopherson]

On Wed, Aug 28, 2019 at 05:02:57PM +0000, Jan Dakinevich wrote:
> exception_type() function was moved for upcoming sanity check in
> emulation code. exceptions_class() function is not supposed to be used
> right now, but it was moved as well to keep things together.

Doh, I didn't realize exception_type() was confined to x86.c when I
suggested the sanity check.  It'd probably be better to add the check
in x86_emulate_instruction and forego this patch, e.g.:

	if (ctxt->have_exception) {
		WARN_ON_ONCE(...);
		inject_emulated_exception(vcpu));
		return EMULATE_DONE;
	}

Arguably we shouldn't WARN on an unexpected vector until we actually try
to inject it anyways.

Sorry for the thrash.

> 
> Cc: Denis Lunev <den@virtuozzo.com>
> Cc: Roman Kagan <rkagan@virtuozzo.com>
> Cc: Denis Plotnikov <dplotnikov@virtuozzo.com>
> Signed-off-by: Jan Dakinevich <jan.dakinevich@virtuozzo.com>
> ---
>  arch/x86/kvm/x86.c | 46 ----------------------------------------------
>  arch/x86/kvm/x86.h | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 903fb7c..2b69ae0 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -364,52 +364,6 @@ asmlinkage __visible void kvm_spurious_fault(void)
>  }
>  EXPORT_SYMBOL_GPL(kvm_spurious_fault);
>  
> -#define EXCPT_BENIGN		0
> -#define EXCPT_CONTRIBUTORY	1
> -#define EXCPT_PF		2
> -
> -static int exception_class(int vector)
> -{
> -	switch (vector) {
> -	case PF_VECTOR:
> -		return EXCPT_PF;
> -	case DE_VECTOR:
> -	case TS_VECTOR:
> -	case NP_VECTOR:
> -	case SS_VECTOR:
> -	case GP_VECTOR:
> -		return EXCPT_CONTRIBUTORY;
> -	default:
> -		break;
> -	}
> -	return EXCPT_BENIGN;
> -}
> -
> -#define EXCPT_FAULT		0
> -#define EXCPT_TRAP		1
> -#define EXCPT_ABORT		2
> -#define EXCPT_INTERRUPT		3
> -
> -static int exception_type(int vector)
> -{
> -	unsigned int mask;
> -
> -	if (WARN_ON(vector > 31 || vector == NMI_VECTOR))
> -		return EXCPT_INTERRUPT;
> -
> -	mask = 1 << vector;
> -
> -	/* #DB is trap, as instruction watchpoints are handled elsewhere */
> -	if (mask & ((1 << DB_VECTOR) | (1 << BP_VECTOR) | (1 << OF_VECTOR)))
> -		return EXCPT_TRAP;
> -
> -	if (mask & ((1 << DF_VECTOR) | (1 << MC_VECTOR)))
> -		return EXCPT_ABORT;
> -
> -	/* Reserved exceptions will result in fault */
> -	return EXCPT_FAULT;
> -}
> -
>  void kvm_deliver_exception_payload(struct kvm_vcpu *vcpu)
>  {
>  	unsigned nr = vcpu->arch.exception.nr;
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index b5274e2..2b66347 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -369,4 +369,50 @@ static inline bool kvm_pat_valid(u64 data)
>  void kvm_load_guest_xcr0(struct kvm_vcpu *vcpu);
>  void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu);
>  
> +#define EXCPT_BENIGN		0
> +#define EXCPT_CONTRIBUTORY	1
> +#define EXCPT_PF		2
> +
> +static inline int exception_class(int vector)
> +{
> +	switch (vector) {
> +	case PF_VECTOR:
> +		return EXCPT_PF;
> +	case DE_VECTOR:
> +	case TS_VECTOR:
> +	case NP_VECTOR:
> +	case SS_VECTOR:
> +	case GP_VECTOR:
> +		return EXCPT_CONTRIBUTORY;
> +	default:
> +		break;
> +	}
> +	return EXCPT_BENIGN;
> +}
> +
> +#define EXCPT_FAULT		0
> +#define EXCPT_TRAP		1
> +#define EXCPT_ABORT		2
> +#define EXCPT_INTERRUPT		3
> +
> +static inline int exception_type(int vector)
> +{
> +	unsigned int mask;
> +
> +	if (WARN_ON(vector > 31 || vector == NMI_VECTOR))
> +		return EXCPT_INTERRUPT;
> +
> +	mask = 1 << vector;
> +
> +	/* #DB is trap, as instruction watchpoints are handled elsewhere */
> +	if (mask & ((1 << DB_VECTOR) | (1 << BP_VECTOR) | (1 << OF_VECTOR)))
> +		return EXCPT_TRAP;
> +
> +	if (mask & ((1 << DF_VECTOR) | (1 << MC_VECTOR)))
> +		return EXCPT_ABORT;
> +
> +	/* Reserved exceptions will result in fault */
> +	return EXCPT_FAULT;
> +}
> +
>  #endif
> -- 
> 2.1.4
>