Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> Bug the VM, i.e. kill it, if the emulator accesses a non-existent GPR,
> i.e. generates an out-of-bounds GPR index. Continuing on all but
> gaurantees some form of data corruption in the guest, e.g. even if KVM
> were to redirect to a dummy register, KVM would be incorrectly read zeros
> and drop writes.
>
> Note, bugging the VM doesn't completely prevent data corruption, e.g. the
> current round of emulation will complete before the vCPU bails out to
> userspace. But, the very act of killing the guest can also cause data
> corruption, e.g. due to lack of file writeback before termination, so
> taking on additional complexity to cleanly bail out of the emulator isn't
> justified, the goal is purely to stem the bleeding and alert userspace
> that something has gone horribly wrong, i.e. to avoid _silent_ data
> corruption.
Thanks, I agree wholeheartedly :-)
>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/kvm/emulate.c | 4 ++--
> arch/x86/kvm/kvm_emulate.h | 10 ++++++++++
> arch/x86/kvm/x86.c | 9 +++++++++
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 77161f57c8d3..70a8e0cd9fdc 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -247,7 +247,7 @@ enum x86_transfer_type {
>
> static ulong reg_read(struct x86_emulate_ctxt *ctxt, unsigned nr)
> {
> - if (WARN_ON_ONCE(nr >= NR_EMULATOR_GPRS))
> + if (KVM_EMULATOR_BUG_ON(nr >= NR_EMULATOR_GPRS, ctxt))
> nr &= NR_EMULATOR_GPRS - 1;
>
> if (!(ctxt->regs_valid & (1 << nr))) {
> @@ -259,7 +259,7 @@ static ulong reg_read(struct x86_emulate_ctxt *ctxt, unsigned nr)
>
> static ulong *reg_write(struct x86_emulate_ctxt *ctxt, unsigned nr)
> {
> - if (WARN_ON_ONCE(nr >= NR_EMULATOR_GPRS))
> + if (KVM_EMULATOR_BUG_ON(nr >= NR_EMULATOR_GPRS, ctxt))
> nr &= NR_EMULATOR_GPRS - 1;
>
> BUILD_BUG_ON(sizeof(ctxt->regs_dirty) * BITS_PER_BYTE < NR_EMULATOR_GPRS);
> diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h
> index 034c845b3c63..89246446d6aa 100644
> --- a/arch/x86/kvm/kvm_emulate.h
> +++ b/arch/x86/kvm/kvm_emulate.h
> @@ -89,6 +89,7 @@ struct x86_instruction_info {
> #define X86EMUL_INTERCEPTED 6 /* Intercepted by nested VMCB/VMCS */
>
> struct x86_emulate_ops {
> + void (*vm_bugged)(struct x86_emulate_ctxt *ctxt);
> /*
> * read_gpr: read a general purpose register (rax - r15)
> *
> @@ -383,6 +384,15 @@ struct x86_emulate_ctxt {
> bool is_branch;
> };
>
> +#define KVM_EMULATOR_BUG_ON(cond, ctxt) \
> +({ \
> + int __ret = (cond); \
> + \
> + if (WARN_ON_ONCE(__ret)) \
> + ctxt->ops->vm_bugged(ctxt); \
> + unlikely(__ret); \
> +})
> +
> /* Repeat String Operation Prefix */
> #define REPE_PREFIX 0xf3
> #define REPNE_PREFIX 0xf2
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7460b9a77d9a..e60badfbbc42 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7887,7 +7887,16 @@ static int emulator_set_xcr(struct x86_emulate_ctxt *ctxt, u32 index, u64 xcr)
> return __kvm_set_xcr(emul_to_vcpu(ctxt), index, xcr);
> }
>
> +static void emulator_vm_bugged(struct x86_emulate_ctxt *ctxt)
> +{
> + struct kvm *kvm = emul_to_vcpu(ctxt)->kvm;
> +
> + if (!kvm->vm_bugged)
> + kvm_vm_bugged(kvm);
> +}
> +
> static const struct x86_emulate_ops emulate_ops = {
> + .vm_bugged = emulator_vm_bugged,
> .read_gpr = emulator_read_gpr,
> .write_gpr = emulator_write_gpr,
> .read_std = emulator_read_std,
Is it actually "vm_bugged" or "kvm_bugged"? :-)
Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
--
Vitaly
