KVM: VMX: Don't register posted interrupt wakeup handler if alloc_kvm_area() fails

[PATCH] KVM: VMX: Don't register posted interrupt wakeup handler if alloc_kvm_area() fails

Posted by Hou Wenlong 3 weeks, 4 days ago

Unregistering the posted interrupt wakeup handler only happens during
hardware unsetup. Therefore, if alloc_kvm_area() fails and continue to
register the posted interrupt wakeup handler, this will leave the global
posted interrupt wakeup handler pointer in an incorrect state. Although
it should not be an issue, it's still better to change it.

Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
---
 arch/x86/kvm/vmx/vmx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9b92f672ccfe..676f32aa72bb 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8829,8 +8829,11 @@ __init int vmx_hardware_setup(void)
 	}
 
 	r = alloc_kvm_area();
-	if (r && nested)
-		nested_vmx_hardware_unsetup();
+	if (r) {
+		if (nested)
+			nested_vmx_hardware_unsetup();
+		return r;
+	}
 
 	kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
 

base-commit: f62b64b970570c92fe22503b0cdc65be7ce7fc7c
-- 
2.31.1

Re: [PATCH] KVM: VMX: Don't register posted interrupt wakeup handler if alloc_kvm_area() fails

Posted by Sean Christopherson 3 weeks, 2 days ago

On Tue, 13 Jan 2026 19:56:50 +0800, Hou Wenlong wrote:
> Unregistering the posted interrupt wakeup handler only happens during
> hardware unsetup. Therefore, if alloc_kvm_area() fails and continue to
> register the posted interrupt wakeup handler, this will leave the global
> posted interrupt wakeup handler pointer in an incorrect state. Although
> it should not be an issue, it's still better to change it.
> 
> 
> [...]

Applied to kvm-x86 vmx, with the goto approach.  Thanks!

[1/1] KVM: VMX: Don't register posted interrupt wakeup handler if alloc_kvm_area() fails
      https://github.com/kvm-x86/linux/commit/6c8512a5b7f4

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH] KVM: VMX: Don't register posted interrupt wakeup handler if alloc_kvm_area() fails

Posted by Sean Christopherson 3 weeks, 4 days ago

On Tue, Jan 13, 2026, Hou Wenlong wrote:
> Unregistering the posted interrupt wakeup handler only happens during
> hardware unsetup. Therefore, if alloc_kvm_area() fails and continue to
> register the posted interrupt wakeup handler, this will leave the global
> posted interrupt wakeup handler pointer in an incorrect state. Although
> it should not be an issue, it's still better to change it.

Ouch, yeah, that's ugly.  It's not entirely benign, as a failed allocation followed
by a spurious notification vector IRQ would trigger UAF.  So it's probably worth
adding:

  Fixes: ec5a4919fa7b ("KVM: VMX: Unregister posted interrupt wakeup handler on hardware unsetup")
  Cc: stable@vger.kernel.org

even though I agree it's extremely unlikely to be an issue in practice.

> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9b92f672ccfe..676f32aa72bb 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8829,8 +8829,11 @@ __init int vmx_hardware_setup(void)
>  	}
>  
>  	r = alloc_kvm_area();
> -	if (r && nested)
> -		nested_vmx_hardware_unsetup();
> +	if (r) {
> +		if (nested)
> +			nested_vmx_hardware_unsetup();
> +		return r;
> +	}

I'm leaning towards using a goto with an explicit "return 0" in the happy case,
to make it less likely that a similar bug is introduced in the future.  Any
preference on your end?

E.g. (untested)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9b92f672ccfe..cecaaeb3f82a 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -8829,8 +8829,8 @@ __init int vmx_hardware_setup(void)
        }
 
        r = alloc_kvm_area();
-       if (r && nested)
-               nested_vmx_hardware_unsetup();
+       if (r)
+               goto err_kvm_area;
 
        kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
 
@@ -8857,6 +8857,11 @@ __init int vmx_hardware_setup(void)
 
        kvm_caps.inapplicable_quirks &= ~KVM_X86_QUIRK_IGNORE_GUEST_PAT;
 
+       return 0;
+
+err_kvm_area:
+       if (nested)
+               nested_vmx_hardware_unsetup();
        return r;
 }

Re: [PATCH] KVM: VMX: Don't register posted interrupt wakeup handler if alloc_kvm_area() fails

Posted by Hou Wenlong 3 weeks, 4 days ago

On Tue, Jan 13, 2026 at 08:17:23AM -0800, Sean Christopherson wrote:
> On Tue, Jan 13, 2026, Hou Wenlong wrote:
> > Unregistering the posted interrupt wakeup handler only happens during
> > hardware unsetup. Therefore, if alloc_kvm_area() fails and continue to
> > register the posted interrupt wakeup handler, this will leave the global
> > posted interrupt wakeup handler pointer in an incorrect state. Although
> > it should not be an issue, it's still better to change it.
> 
> Ouch, yeah, that's ugly.  It's not entirely benign, as a failed allocation followed
> by a spurious notification vector IRQ would trigger UAF.  So it's probably worth
> adding:
> 
>   Fixes: ec5a4919fa7b ("KVM: VMX: Unregister posted interrupt wakeup handler on hardware unsetup")
>   Cc: stable@vger.kernel.org
>
Actually, I'm not sure which commit is better as the fix tag:
'bf9f6ac8d749' or 'ec5a4919fa7b'. Before commit 'ec5a4919fa7b', the
handler was registered before alloc_kvm_areas() and was not unregistered
if alloc_kvm_areas() failed. However, it seems my commit message
description is more suitable for fixing 'ec5a4919fa7b'.

> even though I agree it's extremely unlikely to be an issue in practice.
> 
> > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> > ---
> >  arch/x86/kvm/vmx/vmx.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 9b92f672ccfe..676f32aa72bb 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -8829,8 +8829,11 @@ __init int vmx_hardware_setup(void)
> >  	}
> >  
> >  	r = alloc_kvm_area();
> > -	if (r && nested)
> > -		nested_vmx_hardware_unsetup();
> > +	if (r) {
> > +		if (nested)
> > +			nested_vmx_hardware_unsetup();
> > +		return r;
> > +	}
> 
> I'm leaning towards using a goto with an explicit "return 0" in the happy case,
> to make it less likely that a similar bug is introduced in the future.  Any
> preference on your end?
> 
I don't have a strong preference either way. However, I agree that using
a goto statement could help prevent potential bugs in the future. Do I
need to send a v2?

Thanks!

> E.g. (untested)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9b92f672ccfe..cecaaeb3f82a 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8829,8 +8829,8 @@ __init int vmx_hardware_setup(void)
>         }
>  
>         r = alloc_kvm_area();
> -       if (r && nested)
> -               nested_vmx_hardware_unsetup();
> +       if (r)
> +               goto err_kvm_area;
>  
>         kvm_set_posted_intr_wakeup_handler(pi_wakeup_handler);
>  
> @@ -8857,6 +8857,11 @@ __init int vmx_hardware_setup(void)
>  
>         kvm_caps.inapplicable_quirks &= ~KVM_X86_QUIRK_IGNORE_GUEST_PAT;
>  
> +       return 0;
> +
> +err_kvm_area:
> +       if (nested)
> +               nested_vmx_hardware_unsetup();
>         return r;
>  }

Re: [PATCH] KVM: VMX: Don't register posted interrupt wakeup handler if alloc_kvm_area() fails

Posted by Sean Christopherson 3 weeks, 3 days ago

On Wed, Jan 14, 2026, Hou Wenlong wrote:
> On Tue, Jan 13, 2026 at 08:17:23AM -0800, Sean Christopherson wrote:
> > On Tue, Jan 13, 2026, Hou Wenlong wrote:
> > > Unregistering the posted interrupt wakeup handler only happens during
> > > hardware unsetup. Therefore, if alloc_kvm_area() fails and continue to
> > > register the posted interrupt wakeup handler, this will leave the global
> > > posted interrupt wakeup handler pointer in an incorrect state. Although
> > > it should not be an issue, it's still better to change it.
> > 
> > Ouch, yeah, that's ugly.  It's not entirely benign, as a failed allocation followed
> > by a spurious notification vector IRQ would trigger UAF.  So it's probably worth
> > adding:
> > 
> >   Fixes: ec5a4919fa7b ("KVM: VMX: Unregister posted interrupt wakeup handler on hardware unsetup")
> >   Cc: stable@vger.kernel.org
> >
> Actually, I'm not sure which commit is better as the fix tag:
> 'bf9f6ac8d749' or 'ec5a4919fa7b'. Before commit 'ec5a4919fa7b', the
> handler was registered before alloc_kvm_areas() and was not unregistered
> if alloc_kvm_areas() failed. However, it seems my commit message
> description is more suitable for fixing 'ec5a4919fa7b'.

Yeah, and the Fixes: chain of <this patch> => ec5a4919fa7b => bf9f6ac8d749 provides
the context needed to get back to the original bug.

> > even though I agree it's extremely unlikely to be an issue in practice.
> > 
> > > Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> > > ---
> > >  arch/x86/kvm/vmx/vmx.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > > index 9b92f672ccfe..676f32aa72bb 100644
> > > --- a/arch/x86/kvm/vmx/vmx.c
> > > +++ b/arch/x86/kvm/vmx/vmx.c
> > > @@ -8829,8 +8829,11 @@ __init int vmx_hardware_setup(void)
> > >  	}
> > >  
> > >  	r = alloc_kvm_area();
> > > -	if (r && nested)
> > > -		nested_vmx_hardware_unsetup();
> > > +	if (r) {
> > > +		if (nested)
> > > +			nested_vmx_hardware_unsetup();
> > > +		return r;
> > > +	}
> > 
> > I'm leaning towards using a goto with an explicit "return 0" in the happy case,
> > to make it less likely that a similar bug is introduced in the future.  Any
> > preference on your end?
> > 
> I don't have a strong preference either way. However, I agree that using
> a goto statement could help prevent potential bugs in the future. Do I
> Thanks!

Nah, I'll change it to a goto when applying.

Thanks!