Re: [PATCH v2 6/8] KVM: x86: Bug the VM if the emulator accesses a non-existent GPR

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> 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@google.com>
> ---
>  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@redhat.com>

-- 
Vitaly