[PATCH v2] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

[PATCH v2] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

[Haoxiang Li]

kvm_s390_gisc_register() registers the guest ISC before pinning
the guest interrupt forwarding pages and allocating the AISB bit.
If any of the later setup steps fails, the function unwinds the
pinned pages and other local state, but does not unregister the
GISC reference. Add the missing kvm_s390_gisc_unregister() to the
error unwind path.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Cc: stable@vger.kernel.org
Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>
---
Changes in v2:
 - Move unregister call after "out" label. Thanks, Matt!
---
 arch/s390/kvm/pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 5b075c38998e..686113be0530 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -328,6 +328,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 unpin1:
 	unpin_user_page(aibv_page);
 out:
+	kvm_s390_gisc_unregister(kvm, fib->fmt0.isc);
 	return rc;
 }
 
-- 
2.25.1

Re: [PATCH v2] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

Pre-existing issues:
- [Critical] If userspace disables summary notifications (`sb == 0`), a bogus physical address derived from a NULL pointer is programmed into the hardware GAITE, leading to hardware memory corruption.
- [High] Failure paths in `kvm_s390_pci_aif_enable()` fail silently by returning 0 (success) instead of a negative error code.
- [High] Unbounded resource leaks can be triggered by calling the KVM_S390_ZPCIOP_REGAEN ioctl repeatedly.
- [High] Missing NULL check for the return value of `airq_iv_create()` causes a guaranteed host kernel panic on memory allocation failure.
- [High] Missing `free_uid()` causes a persistent `user_struct` reference leak in `account_mem()` and `unaccount_mem()`.
- [High] Permanent leak of `locked_vm` memory accounting on error path.
- [Medium] Failure of `kvm_zpci_set_airq()` completely bypasses the error unwind path, temporarily leaking pinned pages and allocated structures.
--

commit a4932a1950ea471069d9ddad4938cb5cea43c9a1
Author: Haoxiang Li <haoxiang_li2024@163.com>

KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

kvm_s390_gisc_register() registers the guest ISC before pinning
the guest interrupt forwarding pages and allocating the AISB bit.
If any of the later setup steps fails, the function unwinds the
pinned pages and other local state, but does not unregister the
GISC reference. Add the missing kvm_s390_gisc_unregister() to the
error unwind path.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5b075c38998e3..686113be05309 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -328,6 +328,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  unpin1:
>  	unpin_user_page(aibv_page);
>  out:
> +	kvm_s390_gisc_unregister(kvm, fib->fmt0.isc);

[Severity: High]
This isn't a regression introduced by this patch, but while reviewing the
error unwind path I noticed a potential leak of the locked_vm accounting:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	/* Account for pinned pages, roll back on failure */
	if (account_mem(pcount))
		goto unpin2;
    ...
unlock:
	mutex_unlock(&aift->aift_lock);
unpin2:
	if (fib->fmt0.sum == 1)
		unpin_user_page(aisb_page);
unpin1:
	unpin_user_page(aibv_page);
out:
    ...
}

Are we missing a call to unaccount_mem() in the error unwind block to
revert the accounting if a later step fails?


[Severity: High]
This is a pre-existing regression, but I also noticed that the return code
doesn't seem to be set to an error value on some of these failure paths:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	/* Account for pinned pages, roll back on failure */
	if (account_mem(pcount))
		goto unpin2;
    ...
	bit = airq_iv_alloc_bit(aift->sbv);
	if (bit == -1UL)
		goto unlock;
    ...
}

If either of these fail, will the function silently return 0 instead of a
negative error code since rc is not updated before the goto?


[Severity: Medium]
This isn't a regression introduced by this commit, but does the hardware
setup step bypass this unwind path completely?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	/* Issue the clp to setup the irq now */
	rc = kvm_zpci_set_airq(zdev);
	return rc;

unlock:
    ...
}

If kvm_zpci_set_airq() fails, returning directly abandons the unwind
protocol and leaves the allocated resources behind.


[Severity: Critical]
This is another pre-existing regression, but I have a question about how we
handle a missing guest AISB:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	gaite->aisb = virt_to_phys(page_address(aisb_page) + (fib->fmt0.aisb &
							      ~PAGE_MASK));
    ...
}

If userspace disables summary notifications (fib->fmt0.sum == 0),
aisb_page will be NULL. Does this pointer arithmetic generate a bogus
physical address that gets programmed into the hardware GAITE?


[Severity: High]
This isn't a regression introduced by this patch, but I noticed a potential
null pointer dereference:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
				    AIRQ_IV_BITLOCK |
				    AIRQ_IV_GUESTVEC,
				    phys_to_virt(fib->fmt0.aibv));
    ...
}

arch/s390/kvm/pci.c:kvm_zpci_set_airq() {
    ...
	fib.fmt0.noi = airq_iv_end(zdev->aibv);
    ...
}

If airq_iv_create() fails and returns NULL under memory pressure, will
airq_iv_end() unconditionally dereference the NULL pointer?


[Severity: High]
This isn't a regression introduced here, but I have a question about the
ioctl handler:

arch/s390/kvm/pci.c:kvm_s390_pci_zpci_reg_aen() {
    ...
	return kvm_s390_pci_aif_enable(zdev, &fib, hostflag);
}

If a user repeatedly issues the KVM_S390_ZPCIOP_REGAEN ioctl, does this
unconditionally call kvm_s390_pci_aif_enable() and overwrite zdev->aisb
and zdev->aibv without checking or freeing previous allocations?


[Severity: High]
This isn't a regression introduced by this patch, but there appears to be a
reference leak in the memory accounting functions:

arch/s390/kvm/pci.c:account_mem() {
    ...
	struct user_struct *user = get_uid(current_user());
    ...
}

Does get_uid() increment the reference count without a corresponding call
to free_uid() before the function returns? This also appears to happen in
unaccount_mem().

>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624061910.2794734-1-haoxiang_li2024@163.com?part=1

Re: [PATCH v2] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

[Claudio Imbrenda]

On Wed, 24 Jun 2026 14:19:10 +0800
Haoxiang Li <haoxiang_li2024@163.com> wrote:

> kvm_s390_gisc_register() registers the guest ISC before pinning
> the guest interrupt forwarding pages and allocating the AISB bit.
> If any of the later setup steps fails, the function unwinds the
> pinned pages and other local state, but does not unregister the
> GISC reference. Add the missing kvm_s390_gisc_unregister() to the
> error unwind path.
> 
> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
> Cc: stable@vger.kernel.org
> Signed-off-by: Haoxiang Li <haoxiang_li2024@163.com>

Acked-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> ---
> Changes in v2:
>  - Move unregister call after "out" label. Thanks, Matt!
> ---
>  arch/s390/kvm/pci.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5b075c38998e..686113be0530 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -328,6 +328,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  unpin1:
>  	unpin_user_page(aibv_page);
>  out:
> +	kvm_s390_gisc_unregister(kvm, fib->fmt0.isc);
>  	return rc;
>  }
>