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 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Lack of guest-provided AIBV/AISB address boundary validation allows arbitrary host memory corruption.
- [High] Missing cleanup on KVM_S390_ZPCIOP_REG_AEN overwrite and failure paths causes resource leaks and silent failures.
- [High] Missing NULL pointer check for airq_iv_create() return value causes a host kernel panic.
- [High] Reference count leak of user_struct and unsafe memory un-accounting context corrupts memory quotas.
--

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

KVM: s390: pci: Fix GISC refcount leak on AIF enable failure
    
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.

> 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);

[Severity: High]
This isn't a bug introduced by this patch, but does this function properly
handle failure paths and repeated calls?

If a guest repeatedly issues the KVM_S390_ZPCIOP_REG_AEN ioctl, is there a
check to prevent blindly overwriting existing allocations?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    zdev->aibv = airq_iv_create(...);
    ...
}

Also, if airq_iv_alloc_bit() fails, it appears we jump to unlock without
setting a negative rc or calling unaccount_mem(pcount):

    bit = airq_iv_alloc_bit(aift->sbv);
    if (bit == -1UL)
        goto unlock;

Additionally, if kvm_zpci_set_airq() fails, it looks like we return the error
without rolling back the pinned pages, adapter event bits, or memory
accounting:

    rc = kvm_zpci_set_airq(zdev);
    return rc;

Could these paths lead to resource leaks or silent failures if the hardware
rejects the registration?

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

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
                                AIRQ_IV_BITLOCK |
                                AIRQ_IV_GUESTVEC,
                                phys_to_virt(fib->fmt0.aibv));
    ...
}

If the host is under memory pressure and airq_iv_create() fails, we
immediately assign NULL to zdev->aibv. Then in kvm_zpci_set_airq(), it looks
like this is unconditionally dereferenced:

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

Can this lead to a host kernel panic if guest userspace triggers the
registration ioctl?

[Severity: Critical]
This is a pre-existing issue, but are the bounds of the guest-provided
addresses adequately validated here?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
    ...
    gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
    fib->fmt0.aibv = gaddr;
    ...
}

The code pins exactly 1 page. However, could msi_vecs require more space than
what remains in the page from the provided offset? 

If a guest provides an unaligned physical address near the end of a page, and
the interrupt vectors require more space (up to 256 bytes), might the
hardware's DMA write cross the page boundary into unpinned host physical
memory?

[Severity: High]
This isn't a bug introduced by this patch, but how do the memory accounting
functions handle the user_struct reference count and quotas?

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

It seems we increment the user_struct refcount with get_uid() but never free
it, nor do we store the user pointer for accurate future teardown. 

Then in unaccount_mem():

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

    if (user)
        atomic_long_sub(nr_pages, &user->locked_vm);
    ...
}

Does this leak a user_struct reference on each call? Furthermore, if teardown
happens in a different context (such as a kernel thread during hot-unplug),
could it subtract quotas from an unrelated process instead of the process that
originally registered the AEN?

>  	return rc;
>  }

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