Re: [PATCH] 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] 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