Re: [PATCH 30/45] KVM: x86/emulate: Propagate memory faults to userspace

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] KVM: x86: x86_emulate_instruction() fails to exit on EMULATION_MEMORY_FAULT
- [Medium] KVM: x86: handle X86EMUL_MEMORY_FAULT in kvm_handle_memory_failure()
- [Medium] KVM: x86: emulator_read_write_onepage() faults on MMIO with mem attributes
--

commit bb98c6118897ded1d558925a185b73b85f254506
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: x86/emulate: Propagate memory faults to userspace

This commit introduces X86EMUL_MEMORY_FAULT and EMULATION_MEMORY_FAULT to
propagate guest memory attribute faults through the x86 instruction emulator
and out to userspace as -EFAULT or an emulation failure.

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index caff8534267db..5f7f87ff4d17c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4865,7 +4865,6 @@ static int kvm_read_guest_virt_helper(gva_t addr, void *val, unsigned int bytes,
>  {
>  	struct kvm_pagewalk *gva_walk = &vcpu->arch.gva_walk;
>  	void *data = val;
> -	int r = X86EMUL_CONTINUE;
>  
>  	while (bytes) {
>  		gpa_t gpa = gva_walk->gva_to_gpa(vcpu, gva_walk, addr, access, exception);
> @@ -4878,16 +4877,16 @@ static int kvm_read_guest_virt_helper(gva_t addr, void *val, unsigned int bytes,
>  		ret = kvm_vcpu_read_guest_page(vcpu, gpa >> PAGE_SHIFT, data,
>  					       offset, toread);
>  		if (ret < 0) {
> -			r = X86EMUL_IO_NEEDED;
> -			goto out;
> +			if (kvm_prepare_memory_fault_exit_for_range(vcpu, gpa, toread, false))
> +				return X86EMUL_MEMORY_FAULT;
> +			return X86EMUL_IO_NEEDED;
>  		}
>  
>  		bytes -= toread;
>  		data += toread;
>  		addr += toread;
>  	}
> -out:
> -	return r;
> +	return X86EMUL_CONTINUE;
>  }
[ ... ]
> @@ -4969,16 +4975,16 @@ static int kvm_write_guest_virt_helper(gva_t addr, void *val, unsigned int bytes
>  			return X86EMUL_PROPAGATE_FAULT;
>  		ret = kvm_vcpu_write_guest(vcpu, gpa, data, towrite);
>  		if (ret < 0) {
> -			r = X86EMUL_IO_NEEDED;
> -			goto out;
> +			if (kvm_prepare_memory_fault_exit_for_range(vcpu, gpa, towrite, true))
> +				return X86EMUL_MEMORY_FAULT;
> +			return X86EMUL_IO_NEEDED;
>  		}

[Severity: Medium]
Should kvm_handle_memory_failure() in arch/x86/kvm/x86.c also check for
X86EMUL_MEMORY_FAULT and return -EFAULT?

When a guest executes INVPCID, VMXON, VMCLEAR, VMPTRLD, VMREAD, VMWRITE,
VMPTRST, INVEPT, or INVVPID with a memory operand backed by a page that has
KVM_MEMORY_ATTRIBUTE_NR or KVM_MEMORY_ATTRIBUTE_NW set,
kvm_read_guest_virt() or kvm_write_guest_virt_system() calls
kvm_prepare_memory_fault_exit_for_range() (setting vcpu->run->exit_reason to
KVM_EXIT_MEMORY_FAULT) and returns X86EMUL_MEMORY_FAULT.

Callers such as kvm_handle_invpcid() in arch/x86/kvm/x86.c and
nested_vmx_get_vmptr(), handle_vmread(), handle_vmwrite(), handle_vmptrst(),
handle_invept(), and handle_invvpid() in arch/x86/kvm/vmx/nested.c pass that
return value to kvm_handle_memory_failure():

arch/x86/kvm/x86.c:kvm_handle_memory_failure() {
	if (r == X86EMUL_PROPAGATE_FAULT) {
		if (KVM_BUG_ON(!e, vcpu->kvm))
			return -EIO;

		kvm_inject_emulated_page_fault(vcpu, e);
		return 1;
	}

	kvm_prepare_emulation_failure_exit(vcpu);

	return 0;
}

Because kvm_handle_memory_failure() does not handle X86EMUL_MEMORY_FAULT, it
calls kvm_prepare_emulation_failure_exit(), overwriting the prepared
KVM_EXIT_MEMORY_FAULT in vcpu->run with KVM_EXIT_INTERNAL_ERROR and returning
0 instead of -EFAULT.

Similarly, in handle_ud() in arch/x86/kvm/x86.c, if force_emulation_prefix is
enabled and reading the 5-byte prefix via kvm_read_guest_virt() hits a
KVM_MEMORY_ATTRIBUTE_NR page and returns X86EMUL_MEMORY_FAULT, won't
handle_ud() ignore the prepared memory fault exit and fall through to inject
a #UD into the guest instead of returning -EFAULT?

[ ... ]
> @@ -5151,8 +5159,14 @@ static int emulator_read_write_onepage(unsigned long addr, void *val,
>  	 * i.e. if accessing host user memory failed, but this has been KVM's
>  	 * historical ABI for decades.
>  	 */
> -	if (!ret && ops->read_write_guest(vcpu, gpa, val, bytes))
> -		return X86EMUL_CONTINUE;
> +	if (!ret) {
> +		ret = ops->read_write_guest(vcpu, gpa, val, bytes);
> +		if (!ret)
> +			return X86EMUL_CONTINUE;
> +	}
> +
> +	if (kvm_prepare_memory_fault_exit_for_range(vcpu, gpa, bytes, write))
> +		return X86EMUL_MEMORY_FAULT;

[Severity: Medium]
What happens here in emulator_read_write_onepage() when a guest accesses an
emulated MMIO region (such as the local APIC at APIC_DEFAULT_PHYS_BASE or a
GPA with no memslot) while KVM_MEMORY_ATTRIBUTE_NR or KVM_MEMORY_ATTRIBUTE_NW
is set on that GPA range?

In arch/x86/kvm/mmu/mmu.c, both kvm_mmu_faultin_pfn() and
kvm_faultin_memory_protections() skip memory attribute checks when !slot,
allowing MMIO SPTE creation and returning RET_PF_EMULATE.

However, in emulator_read_write_onepage(),
kvm_prepare_memory_fault_exit_for_range() is called both when ret == 1 (the
GPA was already identified as MMIO by vcpu_is_mmio_gpa() or
vcpu_mmio_gva_to_gpa()) and when ops->read_write_guest() fails due to a
missing memslot.

Because kvm_prepare_memory_fault_exit_for_range() in virt/kvm/kvm_main.c
checks kvm_mem_attributes_may_read_gfn() and
kvm_mem_attributes_may_write_gfn() without checking whether a memslot exists
for the GFN, won't it return X86EMUL_MEMORY_FAULT and abort MMIO emulation
before reaching ops->read_write_mmio()?

[ ... ]
> @@ -6528,7 +6546,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
>  	if (r == EMULATION_INTERCEPTED)
>  		return 1;
>  
> -	if (r == EMULATION_FAILED) {
> +	if (r == EMULATION_MEMORY_FAULT || r == EMULATION_FAILED) {
>  		if (kvm_unprotect_and_retry_on_failure(vcpu, cr2_or_gpa,
>  						       emulation_type))
>  			return 1;

[Severity: High]
When x86_emulate_insn() returns EMULATION_MEMORY_FAULT in
x86_emulate_instruction(), routing it through
kvm_unprotect_and_retry_on_failure() and handle_emulation_failure() seems to
prevent the memory fault from reaching userspace in several cases:

arch/x86/kvm/x86.c:handle_emulation_failure() {
	...
	if (emulation_type & EMULTYPE_VMWARE_GP) {
		kvm_queue_exception_e(vcpu, GP_VECTOR, 0);
		return 1;
	}

	if (kvm->arch.exit_on_emulation_error ||
	    (emulation_type & EMULTYPE_SKIP)) {
		prepare_emulation_ctxt_failure_exit(vcpu);
		return 0;
	}

	kvm_queue_exception(vcpu, UD_VECTOR);

	if (!is_guest_mode(vcpu) && kvm_x86_call(get_cpl)(vcpu) == 0) {
		prepare_emulation_ctxt_failure_exit(vcpu);
		return 0;
	}

	return 1;
}

1. If EMULTYPE_ALLOW_RETRY_PF is set, kvm_unprotect_and_retry_on_failure()
   will unprotect the shadow page and return true, causing
   x86_emulate_instruction() to return 1 and re-enter the guest instead of
   exiting to userspace.

2. If kvm->arch.exit_on_emulation_error is false (the default) and
   !(emulation_type & EMULTYPE_SKIP), handle_emulation_failure() queues a #UD
   into the guest and, whenever is_guest_mode(vcpu) is true or
   get_cpl(vcpu) != 0, returns 1 to resume the guest without exiting to
   userspace. In particular, emulator_io_port_access_allowed() in
   arch/x86/kvm/emulate.c is only called when emulator_bad_iopl(ctxt) is true
   (which requires CPL > 0), so returning X86EMUL_MEMORY_FAULT from
   emulator_io_port_access_allowed() will always have get_cpl(vcpu) > 0 and
   inject #UD (or #GP for EMULTYPE_VMWARE_GP) into the guest instead of
   exiting to userspace.

3. Even when handle_emulation_failure() does exit to userspace by returning
   0, prepare_emulation_ctxt_failure_exit() overwrites the
   KVM_EXIT_MEMORY_FAULT prepared by kvm_prepare_memory_fault_exit_for_range()
   with KVM_EXIT_INTERNAL_ERROR, and still queues #UD if
   !kvm->arch.exit_on_emulation_error.

Should EMULATION_MEMORY_FAULT be handled separately from EMULATION_FAILED so
that it always exits to userspace (for example by returning -EFAULT when no
partial state changes have occurred, or preparing the failure exit without
retrying or injecting #UD/#GP into the guest)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102174130.8779-1-pbonzini@redhat.com?part=30