Re: [PATCH 31/45] KVM: x86: "Handle" failed decode when skipping instructions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: x86: skip_emulated_instruction() returns 0 with KVM_EXIT_MEMORY_FAULT
- [Medium] KVM: SVM: __svm_skip_emulated_instruction() kills VM on soft int RIP update
--

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

KVM: x86: "Handle" failed decode when skipping instructions

This commit marks the VM dead when kvm_emulate_instruction() returns a
negative error code while skipping an emulated instruction in SVM and VMX,
and updates kvm_skip_emulated_instruction() and ICEBP handlers to check
for failure.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index ece8d13c18a70..669bf8b36bb63 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -304,6 +304,7 @@ static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  	unsigned long old_rflags;
> +	int r;
>  
>  	/*
>  	 * SEV-ES does not expose the next RIP. The RIP update is controlled by
> @@ -321,7 +322,10 @@ static int __svm_skip_emulated_instruction(struct kvm_vcpu *vcpu,
>  		if (unlikely(!commit_side_effects))
>  			old_rflags = svm->vmcb->save.rflags;
>  
> -		if (!kvm_emulate_instruction(vcpu, emul_type))
> +		r = kvm_emulate_instruction(vcpu, emul_type);
> +		if (r < 0)
> +			kvm_vm_dead(vcpu->kvm);
> +		if (r <= 0)
>  			return 0;
>  
>  		if (unlikely(!commit_side_effects))

[Severity: Medium]
Should __svm_skip_emulated_instruction() really mark the VM dead when called
from svm_update_soft_interrupt_rip()?

Prior to VM-Entry, kvm_check_and_inject_events() invokes
svm_inject_exception() or svm_inject_irq() -> svm_update_soft_interrupt_rip()
to compute NextRIP before injecting a software exception or software
interrupt:

arch/x86/kvm/svm/svm.c:svm_update_soft_interrupt_rip() {
	const int emul_type = EMULTYPE_SKIP | EMULTYPE_SKIP_SOFT_INT |
			      EMULTYPE_SET_SOFT_INT_VECTOR(vector);
	...
	if (!__svm_skip_emulated_instruction(vcpu, emul_type, !nrips))
		return -EIO;

In that path, no instruction side effects have been performed yet (and when
nrips is enabled, commit_side_effects is false). As noted in the comment in
svm_update_soft_interrupt_rip(), decoding is not guaranteed to succeed, and a
failure returns -EIO so the caller drops the event injection and lets the
guest retry the instruction.

If the guest page at CS:RIP has restrictive memory attributes (such as
KVM_MEMORY_ATTRIBUTE_NX), kvm_emulate_instruction() returns -EFAULT and
__svm_skip_emulated_instruction() now calls kvm_vm_dead(vcpu->kvm),
terminating the VM instead of aborting the injection.

[ ... ]

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index b4a42785ff727..57be93198d335 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1809,6 +1809,7 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	union vmx_exit_reason exit_reason = vt_get_exit_reason(vcpu);
>  	unsigned long rip, orig_rip;
>  	u32 instr_len;
> +	int r;
>  
>  	/*
>  	 * Using VMCS.VM_EXIT_INSTRUCTION_LEN on EPT misconfig depends on
> @@ -1857,7 +1858,10 @@ static int skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  #endif
>  		kvm_rip_write(vcpu, rip);
>  	} else {
> -		if (!kvm_emulate_instruction(vcpu, EMULTYPE_SKIP))
> +		r = kvm_emulate_instruction(vcpu, EMULTYPE_SKIP);
> +		if (r < 0)
> +			kvm_vm_dead(vcpu->kvm);
> +		if (r <= 0)
>  			return 0;
>  	}
>  

[ ... ]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5f7f87ff4d17c..5818101d1d8ae 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6235,8 +6235,8 @@ int kvm_skip_emulated_instruction(struct kvm_vcpu *vcpu)
>  	int r;
>  
>  	r = kvm_x86_call(skip_emulated_instruction)(vcpu);
> -	if (unlikely(!r))
> -		return 0;
> +	if (unlikely(r <= 0))
> +		return r;
>  
>  	kvm_pmu_instruction_retired(vcpu);
>  

[Severity: High]
Won't clamping r < 0 to 0 in __svm_skip_emulated_instruction() and VMX's
skip_emulated_instruction() cause KVM_RUN to return 0 with exit_reason set to
KVM_EXIT_MEMORY_FAULT, while also making the r < 0 case in
kvm_skip_emulated_instruction() unreachable?

When an instruction fetch hits a guest page with non-executable or
non-readable memory attributes during kvm_emulate_instruction() ->
x86_emulate_instruction() -> x86_decode_emulated_instruction() ->
x86_decode_insn() -> kvm_fetch_guest_virt(), kvm_prepare_memory_fault_exit()
sets vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT, and
x86_emulate_instruction() returns -EFAULT before reaching
handle_emulation_failure():

arch/x86/kvm/x86.c:x86_emulate_instruction() {
		r = x86_decode_emulated_instruction(vcpu, emulation_type,
						    insn, insn_len);
		if (r == EMULATION_MEMORY_FAULT)
			return -EFAULT;

In both __svm_skip_emulated_instruction() and VMX's
skip_emulated_instruction(), the check:

		if (r < 0)
			kvm_vm_dead(vcpu->kvm);
		if (r <= 0)
			return 0;

returns 0 when r is negative, so kvm_x86_call(skip_emulated_instruction)(vcpu)
never returns a negative value to kvm_skip_emulated_instruction().

When 0 is propagated back to vcpu_enter_guest() -> vcpu_run() (or through
vcpu->arch.complete_userspace_io in kvm_arch_vcpu_ioctl_run()), the run loop
exits to userspace immediately on r <= 0 without re-entering
vcpu_enter_guest() where KVM_REQ_VM_DEAD is checked and converted into -EIO.
As a result, ioctl(KVM_RUN) returns 0 to userspace with vcpu->run->exit_reason
left as KVM_EXIT_MEMORY_FAULT, violating the KVM_EXIT_MEMORY_FAULT UAPI
contract (which requires a -1 return with errno set to EFAULT or EHWPOISON).
In addition, in callers such as kvm_emulate_halt() and
kvm_emulate_ap_reset_hold() with a userspace irqchip, __kvm_emulate_halt()
overwrites exit_reason with KVM_EXIT_HLT or KVM_EXIT_AP_RESET_HOLD before
returning 0.

Note that if the vendor callbacks are changed to return a negative error code
(such as -EIO) when r < 0, callers that test the return value as a boolean --
such as !svm_skip_emulated_instruction(vcpu) in icebp_interception(),
task_switch_interception(), nested_svm_vmrun(), and
svm_update_soft_interrupt_rip(); !skip_emulated_instruction(vcpu) in
handle_exception_nmi() and handle_task_switch();
!kvm_skip_emulated_instruction(vcpu) in kvm_emulate_wrmsr(); and
ret && kvm_skip_emulated_instruction(vcpu) in kvm_fast_pio(),
kvm_emulate_halt(), and kvm_emulate_ap_reset_hold() -- would also need to
check <= 0 so they do not treat a negative errno as success.

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