Re: [PATCH] KVM: x86: Don't leave APF half-enabled on bad APF data GPA

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 4/3/2026 11:11 AM, Ethan Yang wrote:
> kvm_pv_enable_async_pf() updates vcpu->arch.apf.msr_en_val before
> initializing the APF data gfn_to_hva cache. If userspace provides an
> invalid GPA, kvm_gfn_to_hva_cache_init() fails, but msr_en_val stays
> enabled and leaves APF state half-initialized.
> 
> Later APF paths can then try to use the empty cache and trigger
> WARN_ON() in kvm_read_guest_offset_cached().
> 
> Determine the new APF enabled state from the incoming MSR value, do cache
> initialization first on the enable path, and commit msr_en_val only after
> successful initialization. Keep the disable path behavior unchanged.
> 
> Reported-by: syzbot+bc0e18379a290e5edfe4@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=bc0e18379a290e5edfe4
> Link: https://lore.kernel.org/r/aHfD3MczrDpzDX9O@google.com
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Ethan Yang <ethan.yang.kernel@gmail.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

though some nits below.

> ---
>   arch/x86/kvm/x86.c | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fd1c4a36b59..c355bbd36c3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1041,11 +1041,15 @@ bool kvm_require_dr(struct kvm_vcpu *vcpu, int dr)
>   }
>   EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_require_dr);
>   
> -static bool kvm_pv_async_pf_enabled(struct kvm_vcpu *vcpu)
> +static bool kvm_pv_async_pf_enabled_data(u64 data)

I'm not sure if the name with "__" as __kvm_pv_async_pf_enabled() fits 
better?

>   {
>   	u64 mask = KVM_ASYNC_PF_ENABLED | KVM_ASYNC_PF_DELIVERY_AS_INT;
> +	return (data & mask) == mask;
> +}
>   
> -	return (vcpu->arch.apf.msr_en_val & mask) == mask;
> +static bool kvm_pv_async_pf_enabled(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_pv_async_pf_enabled_data(vcpu->arch.apf.msr_en_val);
>   }
>   
>   static inline u64 pdptr_rsvd_bits(struct kvm_vcpu *vcpu)
> @@ -3645,17 +3649,20 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
>   	if (!lapic_in_kernel(vcpu))
>   		return data ? 1 : 0;
>   
> +	bool new_enabled = kvm_pv_async_pf_enabled_data(data);

I would like just call it "enable"

> +
> +	if (new_enabled &&
> +		kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.apf.data, gpa,

Please align the second line

> +			sizeof(u64)))
> +		return 1;
>   	vcpu->arch.apf.msr_en_val = data;
>   
> -	if (!kvm_pv_async_pf_enabled(vcpu)) {
> +	if (!new_enabled) {
>   		kvm_clear_async_pf_completion_queue(vcpu);
>   		kvm_async_pf_hash_reset(vcpu);
>   		return 0;
>   	}
>   
> -	if (kvm_gfn_to_hva_cache_init(vcpu->kvm, &vcpu->arch.apf.data, gpa,
> -					sizeof(u64)))
> -		return 1;
>   
>   	vcpu->arch.apf.send_always = (data & KVM_ASYNC_PF_SEND_ALWAYS);
>   	vcpu->arch.apf.delivery_as_pf_vmexit = data & KVM_ASYNC_PF_DELIVERY_AS_PF_VMEXIT;