Re: [GIT PULL 1/5] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Repeated calls to the `KVM_S390_ZPCIOP_REG_AEN` ioctl leak resources and cause Use-After-Free.
- [High] `account_mem` and `unaccount_mem` leak `struct user_struct` reference counts and use wrong accounting context.
- [High] `kvm_s390_pci_aif_enable` silently fails on memory accounting or bit allocation failure.
- [High] Resource leak on `kvm_zpci_set_airq` hardware setup failure.
- [High] Missing memory unaccounting on bit allocation failure.
- [High] Unchecked `airq_iv_create()` return value leads to kernel panic.
--

--- Patch [1]: [GIT PULL 1/5] KVM: s390: pci: Fix GISC refcount leak on AIF enable failure ---
commit 2781de8fecc1ddfd2641c260ff20fd3cfb663715
Author: Haoxiang Li <haoxiang_li2024@163.com>
Subject: KVM: s390: pci: Fix GISC refcount leak on AIF enable failure

This commit fixes a reference count leak of the guest ISC in the error
handling path of kvm_s390_pci_aif_enable(). It ensures that if setup fails,
kvm_s390_gisc_unregister() is called to properly release the GISC reference.

> 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);
>  	return rc;
>  }

[Severity: High]
This is a pre-existing issue, but does a failure in kvm_zpci_set_airq() leak
all previously allocated resources?

Looking at the lines just above this diff in kvm_s390_pci_aif_enable():

    rc = kvm_zpci_set_airq(zdev);
    return rc;

unlock:
    mutex_unlock(&aift->aift_lock);

Returning rc directly bypasses the unlock, unpin2, unpin1, and out labels.
This skips all resource cleanup, including the new kvm_s390_gisc_unregister()
call added by this patch.


[Severity: High]
This isn't a bug introduced by this patch, but is there a missing call to
unaccount_mem() in the error path?

Looking at the error handling in kvm_s390_pci_aif_enable():

unlock:
    mutex_unlock(&aift->aift_lock);
unpin2:

If airq_iv_alloc_bit() fails and jumps to unlock, it unwinds the pinned pages
but leaves the memory accounted for, potentially leaking the locked_vm limit.


[Severity: High]
This is a pre-existing issue, but do these error paths return 0 to userspace
instead of an error code?

In kvm_s390_pci_aif_enable(), when memory accounting or bit allocation fails:

    if (account_mem(pcount))
        goto unpin2;
    ...
    bit = airq_iv_alloc_bit(aift->sbv);
    if (bit == -1UL)
        goto unlock;

When jumping to the cleanup labels, rc does not appear to be set to a negative
value like -ENOMEM or -ENOSPC. Since rc is initialized to 0, this would
silently mask the internal failure.


[Severity: Critical]
This isn't a bug introduced by this patch, but what happens if userspace calls
the KVM_S390_ZPCIOP_REG_AEN ioctl multiple times?

In kvm_s390_pci_aif_enable():

    zdev->aisb = bit; /* store the summary bit number */
    zdev->aibv = airq_iv_create(...);
    ...
    aift->kzdev[zdev->aisb] = zdev->kzdev;

It looks like this blindly allocates new resources and overwrites zdev->aibv
and zdev->aisb without cleaning up any previous allocations. The old aisb index
remains populated in aift->kzdev, which might lead to a use-after-free when
the device is unregistered.


[Severity: High]
This is a pre-existing issue, but can airq_iv_create() return NULL here?

In kvm_s390_pci_aif_enable():

    zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA | ...

If it fails due to memory pressure, it seems zdev->aibv becomes NULL. Later,
zdev is passed to kvm_zpci_set_airq() which does this:

    fib.fmt0.noi = airq_iv_end(zdev->aibv);
    fib.fmt0.aibv = virt_to_phys(zdev->aibv->vector);

This unconditionally dereferences the unchecked pointer, potentially causing
a crash.


[Severity: High]
This isn't a bug introduced by this patch, but do account_mem() and
unaccount_mem() leak the user_struct reference?

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

I see get_uid() increments the refcount, but there doesn't appear to be a
matching free_uid() on success or error.

Additionally, in unaccount_mem():

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

Calling current_user() might un-account from a different process or kernel
thread if deregistration happens asynchronously.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727095359.107671-2-borntraeger@de.ibm.com?part=1