Re: [PATCH v2] KVM: x86: Handle unexpected MMIO accesses using master abort semantics

[Paolo Bonzini]

On 13/09/19 03:56, Sean Christopherson wrote:
> Use master abort semantics, i.e. reads return all ones and writes are
> dropped, to handle unexpected MMIO accesses when reading guest memory
> instead of returning X86EMUL_IO_NEEDED, which in turn gets interpreted
> as a guest page fault.
> 
> Emulation of certain instructions, notably VMX instructions, involves
> reading or writing guest memory without going through the emulator.
> These emulation flows are not equipped to handle MMIO accesses as no
> sane and properly functioning guest kernel will target MMIO with such
> instructions, and so simply inject a page fault in response to
> X86EMUL_IO_NEEDED.
> 
> While not 100% correct, using master abort semantics is at least
> sometimes correct, e.g. non-existent MMIO accesses do actually master
> abort, whereas injecting a page fault is always wrong, i.e. the issue
> lies in the physical address domain, not in the virtual to physical
> translation.
> 
> Apply the logic to kvm_write_guest_virt_system() in addition to
> replacing existing #PF logic in kvm_read_guest_virt(), as VMPTRST uses
> the former, i.e. can also leak a host stack address.
> 
> Reported-by: Fuqian Huang <huangfq.daxian@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>

For 5.5, I'll look into doing proper emulation in
vmxon/vmclear/vmptrld/vmptrst emulation.

Paolo

> ---
> 
> v2: Fix the comment for kvm_read_guest_virt_helper().
> 
>  arch/x86/kvm/x86.c | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index b4cfd786d0b6..3da57f137470 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5234,16 +5234,24 @@ int kvm_read_guest_virt(struct kvm_vcpu *vcpu,
>  			       struct x86_exception *exception)
>  {
>  	u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0;
> +	int r;
> +
> +	r = kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access,
> +				       exception);
>  
>  	/*
> -	 * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED
> -	 * is returned, but our callers are not ready for that and they blindly
> -	 * call kvm_inject_page_fault.  Ensure that they at least do not leak
> -	 * uninitialized kernel stack memory into cr2 and error code.
> +	 * FIXME: this should technically call out to userspace to handle the
> +	 * MMIO access, but our callers are not ready for that, so emulate
> +	 * master abort behavior instead, i.e. reads return all ones.
>  	 */
> -	memset(exception, 0, sizeof(*exception));
> -	return kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access,
> -					  exception);
> +	if (r == X86EMUL_IO_NEEDED) {
> +		memset(val, 0xff, bytes);
> +		return 0;
> +	}
> +	if (r == X86EMUL_PROPAGATE_FAULT)
> +		return -EFAULT;
> +	WARN_ON_ONCE(r);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(kvm_read_guest_virt);
>  
> @@ -5317,11 +5325,25 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v
>  int kvm_write_guest_virt_system(struct kvm_vcpu *vcpu, gva_t addr, void *val,
>  				unsigned int bytes, struct x86_exception *exception)
>  {
> +	int r;
> +
>  	/* kvm_write_guest_virt_system can pull in tons of pages. */
>  	vcpu->arch.l1tf_flush_l1d = true;
>  
> -	return kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
> -					   PFERR_WRITE_MASK, exception);
> +	r = kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
> +					PFERR_WRITE_MASK, exception);
> +
> +	/*
> +	 * FIXME: this should technically call out to userspace to handle the
> +	 * MMIO access, but our callers are not ready for that, so emulate
> +	 * master abort behavior instead, i.e. writes are dropped.
> +	 */
> +	if (r == X86EMUL_IO_NEEDED)
> +		return 0;
> +	if (r == X86EMUL_PROPAGATE_FAULT)
> +		return -EFAULT;
> +	WARN_ON_ONCE(r);
> +	return 0;
>  }
>  EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
>  
>