* 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
2026-06-23 14:30 ` Matthew Rosato
1 sibling, 0 replies; 3+ 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] 3+ messages in thread* 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
@ 2026-06-23 14:30 ` Matthew Rosato
1 sibling, 0 replies; 3+ messages in thread
From: Matthew Rosato @ 2026-06-23 14:30 UTC (permalink / raw)
To: Haoxiang Li, alifm, farman, borntraeger, frankja, imbrenda, david,
hca, gor, svens, schnelle
Cc: linux-s390, kvm, linux-kernel, stable
On 6/23/26 3:52 AM, Haoxiang Li 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.
Hi Haoxiang,
Thanks for the fix! A comment below..
>
> 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:
Label 'out' is now unused in this function.
I think you could make this a 1-liner fix by keeping the goto unchanged
and instead just placing the unregister call right after out:
Thanks,
Matt
> return rc;
> }
^ permalink raw reply [flat|nested] 3+ messages in thread