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:
https://lkml.org/lkml/2019/8/28/879
v3:
- do sanity check in caller code
- drop patch, that moved emulation_type() function
Jan Dakinevich (2):
KVM: x86: always stop emulation on page fault
KVM: x86: set ctxt->have_exception in x86_decode_insn()
arch/x86/kvm/emulate.c | 2 ++
arch/x86/kvm/x86.c | 7 ++++++-
2 files changed, 8 insertions(+), 1 deletion(-)
--
2.1.4
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 <[email protected]>
Cc: Roman Kagan <[email protected]>
Cc: Denis Plotnikov <[email protected]>
Signed-off-by: Jan Dakinevich <[email protected]>
---
arch/x86/kvm/emulate.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index bef3c3c..698efb8 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -5416,6 +5416,8 @@ 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)
+ ctxt->have_exception = true;
return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
}
--
2.1.4
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 <[email protected]>
Cc: Roman Kagan <[email protected]>
Cc: Denis Plotnikov <[email protected]>
Signed-off-by: Jan Dakinevich <[email protected]>
---
arch/x86/kvm/x86.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b4cfd78..6bf7b55 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6537,8 +6537,13 @@ 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) {
+ WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR);
+ WARN_ON_ONCE(exception_type(ctxt->exception.vector)
+ == EXCPT_TRAP);
+ 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
On Thu, Aug 29, 2019 at 08:23:18AM +0000, Jan Dakinevich wrote:
> 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")
Commit ids should be at least 12 chars, the full tag should be
Fixes: 6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
You can force this in your git config, e.g.
git config --global core.abbrev 12
Both patches in this series should probably have
Cc: <[email protected]>
Reviewed-and-tested-by: Sean Christopherson <[email protected]>
> Cc: Denis Lunev <[email protected]>
> Cc: Roman Kagan <[email protected]>
> Cc: Denis Plotnikov <[email protected]>
> Signed-off-by: Jan Dakinevich <[email protected]>
> ---
> arch/x86/kvm/x86.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b4cfd78..6bf7b55 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6537,8 +6537,13 @@ 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) {
> + WARN_ON_ONCE(ctxt->exception.vector == UD_VECTOR);
> + WARN_ON_ONCE(exception_type(ctxt->exception.vector)
> + == EXCPT_TRAP);
> + 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
>
On Thu, Aug 29, 2019 at 08:23:20AM +0000, Jan Dakinevich wrote:
> 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 <[email protected]>
> Cc: Roman Kagan <[email protected]>
> Cc: Denis Plotnikov <[email protected]>
> Signed-off-by: Jan Dakinevich <[email protected]>
Same nits as last patch:
Cc: <[email protected]>
Fixes: 6ea6e84309ca ("KVM: x86: inject exceptions produced by x86_decode_insn")
Reviewed-and-tested-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/emulate.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index bef3c3c..698efb8 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -5416,6 +5416,8 @@ 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)
> + ctxt->have_exception = true;
> return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK;
> }
>
> --
> 2.1.4
>