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

[PATCH] 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>
---
 arch/s390/kvm/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 5b075c38998e..a9d5996590e7 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -252,7 +252,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	srcu_read_unlock(&kvm->srcu, idx);
 	if (npages < 1) {
 		rc = -EIO;
-		goto out;
+		goto out_unregister_gisc;
 	}
 	aibv_page = pages[0];
 	pcount++;
@@ -327,6 +327,8 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 		unpin_user_page(aisb_page);
 unpin1:
 	unpin_user_page(aibv_page);
+out_unregister_gisc:
+	kvm_s390_gisc_unregister(kvm, fib->fmt0.isc);
 out:
 	return rc;
 }
-- 
2.25.1

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Potential hardware out-of-bounds access due to missing page boundary/alignment checks for AIBV.
- [High] Calling KVM_S390_ZPCIOP_REG_AEN multiple times permanently leaks previously allocated AIF resources because there is no check if the device is already registered.
- [High] `kvm_s390_pci_aif_enable` silently returns success (0) when memory accounting or `aisb` bit allocation fails, and misses `unaccount_mem` during unwind.
- [High] The result of `airq_iv_create` is never checked for NULL, leading to a guaranteed kernel NULL pointer dereference on allocation failure.
- [High] Missing rollback and resource leak if the final hardware setup `kvm_zpci_set_airq(zdev)` fails.
- [High] `account_mem` and `unaccount_mem` leak a reference to the `user_struct` on every invocation by calling `get_uid()` without a matching `free_uid()`.
--

commit 20ea01515bd667a22d838d6da57d809035f9fe06
Author: Haoxiang Li <haoxiang_li2024@163.com>
Subject: KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

This commit adds a missing call to kvm_s390_gisc_unregister() in the
error unwind path of kvm_s390_pci_aif_enable(). Previously, if setup
failed after registering the guest ISC, the reference was not correctly
released.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5b075c38998e3..a9d5996590e79 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -252,7 +252,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	srcu_read_unlock(&kvm->srcu, idx);

[Severity: High]
This is a pre-existing issue, but is there a missing check at the start of
this function to see if the device is already registered?

If userspace repeatedly issues KVM_S390_ZPCIOP_REG_AEN, it appears this
function will blindly pin new pages and allocate new objects, overwriting
zdev->aibv and zdev->aisb without freeing the old ones. Can this leak host
kernel memory and exhaust guest ISC reference counts?

>  	if (npages < 1) {
>  		rc = -EIO;
> -		goto out;
> +		goto out_unregister_gisc;
>  	}
>  	aibv_page = pages[0];
>  	pcount++;

[Severity: Critical]
This is a pre-existing issue, but does this correctly handle unaligned
guest physical addresses?

The code immediately after this pins exactly one page:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
    ...
}

And then computes the unaligned physical address:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
    ...
}

If the guest provides an unaligned guest physical address such that the
interrupt vector size (msi_vecs bits) spans beyond the end of that single
pinned page, will the hardware write into the adjacent unpinned physical
page?

> @@ -327,6 +327,8 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,

[Severity: High]
This isn't a bug introduced by this patch, but earlier in this function
do the error paths leave rc set to 0?

If account_mem() fails, it jumps to unpin2:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    if (account_mem(pcount))
        goto unpin2;
    ...
}

And if airq_iv_alloc_bit() fails, it jumps to unlock:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    if (bit == -1UL)
        goto unlock;
    ...
}

Since rc is initialized to 0, does this falsely report success to userspace?
Also, if airq_iv_alloc_bit() fails and jumps to unlock, it seems to skip
unaccount_mem(pcount), which could leak the memory accounting.

[Severity: High]
This is a pre-existing issue, but does account_mem() leak a reference
to the user_struct?

Looking at the implementation of account_mem() and unaccount_mem():

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

It appears get_uid() increments the reference count, but there is no
matching free_uid(). Can this overflow the reference count over time?

[Severity: High]
This is a pre-existing issue, but what happens if airq_iv_create() fails
and returns a null pointer?

The result is directly assigned to zdev->aibv without validation. Later,
kvm_zpci_set_airq() is called, which dereferences it:

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

Does this cause a null pointer dereference if the allocation fails under
memory pressure?

[Severity: High]
This is a pre-existing issue, but if kvm_zpci_set_airq() fails, does the
return path leak the resources we just allocated?

If the setup fails, the function returns rc directly:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    rc = kvm_zpci_set_airq(zdev);
    return rc;

unlock:
    ...
}

This bypasses the local error unwind labels (unlock, unpin2, unpin1,
out_unregister_gisc). Should this jump to a cleanup path to free the
pinned pages, the airq_iv structures, and unregister the guest ISC?

>  		unpin_user_page(aisb_page);
>  unpin1:
>  	unpin_user_page(aibv_page);
> +out_unregister_gisc:
> +	kvm_s390_gisc_unregister(kvm, fib->fmt0.isc);
>  out:
>  	return rc;
>  }

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