* Re: [PATCH] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure
2026-06-23 7:52 [PATCH] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure Haoxiang Li
@ 2026-06-23 8:07 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-23 8:07 UTC (permalink / raw)
To: Haoxiang Li
Cc: linux-s390, kvm, Heiko Carstens, Alexander Gordeev, Vasily Gorbik
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
^ permalink raw reply [flat|nested] 2+ messages in thread