Re: [PATCH 5/5] KVM: x86: move asynchronous page fault handling to vcpu_enter_guest()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: x86: stack use-after-free in vcpu_enter_guest() when APF token is 0
--

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

KVM: x86: move asynchronous page fault handling to vcpu_enter_guest()

This commit moves host paravirtualized asynchronous page fault handling out
of the MMU page fault paths and into vcpu_enter_guest() using a new
EXIT_FASTPATH_APF fastpath exit completion and a stack-allocated
kvm_task_sleep_node.

Link: https://patch.msgid.link/auqAsEk9o6buPNiR@google.com

> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 3cb2616d88aca..c01a1cf70d77b 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -163,17 +163,20 @@ bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu);
>  int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>  				u64 fault_address, char *insn, int insn_len);
>  
> -static inline void kvm_handle_host_apf_irqoff(struct kvm_vcpu *vcpu, u32 token)
> +static inline fastpath_t kvm_handle_host_apf_irqoff(struct kvm_task_sleep_node *host_apf_node,
> +						    u32 token)
>  {
> -	vcpu->arch.apf.host_apf_flags = kvm_read_and_reset_apf_flags();
> -	if ((vcpu->arch.apf.host_apf_flags & KVM_PV_REASON_PAGE_NOT_PRESENT) &&
> -	    !kvm_async_pf_queue_task(token, &vcpu->arch.apf.host_apf_node)) {
> -		/*
> -		 * Nothing to do but regular page fault handling must still
> -		 * be suppressed, so leave host_apf_flags nonzero.
> -		 */
> -		vcpu->arch.apf.host_apf_flags = KVM_PV_REASON_PAGE_READY;
> +	u32 host_apf_flags = kvm_read_and_reset_apf_flags();
> +
> +	if (!host_apf_flags)
> +		return EXIT_FASTPATH_NONE;
> +
> +	if (!WARN_ON_ONCE(!(host_apf_flags & KVM_PV_REASON_PAGE_NOT_PRESENT))) {
> +		kvm_async_pf_queue_task(token, host_apf_node);
> +		return EXIT_FASTPATH_APF;
>  	}
> +
> +	return EXIT_FASTPATH_NONE;
>  }
>  
>  void __kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu,

[ ... ]

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 2a26900c06bd0..ffcf6ab622518 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c

[ ... ]

> @@ -8130,9 +8140,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	bool req_int_win =
>  		dm_request_for_irq_injection(vcpu) &&
>  		kvm_cpu_accept_dm_intr(vcpu);
> +	struct kvm_task_sleep_node host_apf_node = {0};
>  	fastpath_t exit_fastpath;
>  	u64 run_flags, debug_ctl;
> -
>  	bool req_immediate_exit = false;
>  
>  	if (kvm_request_pending(vcpu)) {
> @@ -8507,7 +8517,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		fpu_sync_guest_vmexit_xfd_state();
>  
>  	if (exit_fastpath == EXIT_FASTPATH_NONE)
> -		exit_fastpath = kvm_x86_call(handle_exit_irqoff)(vcpu);
> +		exit_fastpath = kvm_x86_call(handle_exit_irqoff)(vcpu, &host_apf_node);
>  
>  	if (vcpu->arch.guest_fpu.xfd_err)
>  		wrmsrq(MSR_IA32_XFD_ERR, 0);
> @@ -8569,6 +8579,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  	if (vcpu->arch.apic_attention)
>  		kvm_lapic_sync_from_vapic(vcpu);
>  
> +	if (exit_fastpath == EXIT_FASTPATH_APF && host_apf_node.token)
> +		kvm_handle_pv_async_pf(&host_apf_node);

[Severity: High]
Can this leave the stack-allocated host_apf_node linked in async_pf_sleepers
after vcpu_enter_guest() returns when token is 0?

In kvm_handle_host_apf_irqoff(), kvm_async_pf_queue_task(token,
host_apf_node) is called without checking whether token is 0 or recording the
boolean return value:

arch/x86/kernel/kvm.c:kvm_async_pf_queue_task() {
	...
	n->token = token;
	n->cpu = smp_processor_id();
	init_swait_queue_head(&n->wq);
	hlist_add_head(&n->link, &b->list);
	raw_spin_unlock(&b->lock);
	return true;
}

If token is 0, kvm_async_pf_queue_task() still inserts &host_apf_node.link
into async_pf_sleepers with host_apf_node.token == 0 and returns true.

Then in vcpu_enter_guest(), the condition:

	if (exit_fastpath == EXIT_FASTPATH_APF && host_apf_node.token)
		kvm_handle_pv_async_pf(&host_apf_node);

evaluates to false because host_apf_node.token is 0, skipping
kvm_handle_pv_async_pf() and returning from vcpu_enter_guest() while
host_apf_node.link remains queued in async_pf_sleepers.

A zero token with KVM_PV_REASON_PAGE_NOT_PRESENT set can occur if L0 is buggy
or untrusted, or if L0 KVM sets KVM_PV_REASON_PAGE_NOT_PRESENT in L1's
apf_reason.flags via kvm_arch_async_page_not_present() and queues
vcpu->arch.exception_vmexit, but exception_vmexit is subsequently cleared
before injection into L1 (for example, when preempted by KVM_APIC_INIT in
vmx_check_nested_events() or svm_check_nested_events() calling
kvm_clear_exception_queue(), or across KVM_GET_VCPU_EVENTS and
KVM_SET_VCPU_EVENTS), leaving KVM_PV_REASON_PAGE_NOT_PRESENT stale in L1's
apf_reason.flags until L2 triggers a regular #PF VM-exit whose faulting
address has lower 32 bits equal to 0.

Previously, kvm_handle_host_apf_irqoff() checked the boolean return value of
kvm_async_pf_queue_task() directly instead of testing host_apf_node.token != 0.

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