Re: [GIT PULL 5/5] KVM: s390: pci: Fix handling of AIF enable without AISB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unchecked return value from `airq_iv_create()` causes a host kernel panic in `kvm_zpci_set_airq()`.
- [Critical] Insufficient page pinning for the Adapter Interrupt Bit Vector (AIBV) allows hardware DMA to write beyond the pinned page into arbitrary host memory.
- [Critical] The `set_bit_inv()` call in the interrupt forwarding path can write outside the bounds of the single pinned AISB page due to an unvalidated page offset and bit index.
- [High] Repeated KVM_S390_ZPCIOP_REG_AEN ioctl calls unconditionally overwrite existing AIF allocations, causing unbounded memory and resource exhaustion.
- [High] Error paths in `kvm_s390_pci_aif_enable()` fail to update `rc`, erroneously returning success to userspace when enabling fails.
- [High] Early error paths in `kvm_s390_pci_aif_enable()` fail to rollback successfully allocated resources like GISC registrations and memory quotas.
- [High] `account_mem()` and `unaccount_mem()` perpetually leak `struct user_struct` reference counts.
- [High] `unaccount_mem()` blindly uses `current_user()` and `current->mm`, risking quota corruption when invoked asynchronously or from a different context.
--

--- Patch [1]: [GIT PULL 5/5] KVM: s390: pci: Fix handling of AIF enable without AISB ---
commit e5fbf43dd647bd07ede442137b445e6821685004
Author: Matthew Rosato <mjrosato@linux.ibm.com>
KVM: s390: pci: Fix handling of AIF enable without AISB

When a guest seeks to register IRQs without a summary bit specified, ensure
that the associated GAITE stores 0 for the guest AISB location instead of an
invalid physical address.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5b075c38998e3..dc748cde4703b 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -300,9 +300,14 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  
>  	gaite->gisc = fib->fmt0.isc;
>  	gaite->count++;
> -	gaite->aisbo = fib->fmt0.aisbo;
> -	gaite->aisb = virt_to_phys(page_address(aisb_page) + (fib->fmt0.aisb &
> -							      ~PAGE_MASK));
> +	if (fib->fmt0.sum == 1) {
> +		gaite->aisbo = fib->fmt0.aisbo;
> +		gaite->aisb = virt_to_phys(page_address(aisb_page) +
> +					   (fib->fmt0.aisb & ~PAGE_MASK));

[Severity: Critical]
This is a pre-existing issue, but does this setup allow for an out-of-bounds
write during interrupt forwarding?

If a guest provides a summary bit address near a page boundary and a large
summary bit offset (up to 255), we might exceed the bounds of the single
pinned page. When a host interrupt arrives, aen_host_forward() executes:

    set_bit_inv(gaite->aisbo, phys_to_virt(gaite->aisb));

Since set_bit_inv() modifies a 64-bit word, could this read and write up to
31 bytes into the adjacent, unpinned virtual page in the host linear mapping?

> +	} else {
> +		gaite->aisbo = 0;
> +		gaite->aisb = 0;
> +	}
>  	aift->kzdev[zdev->aisb] = zdev->kzdev;
>  	spin_unlock_irq(&aift->gait_lock);

[Severity: High]
This is a pre-existing issue, but does repeated invocation of the
KVM_S390_ZPCIOP_REG_AEN ioctl leak AIF allocations?

Looking at kvm_s390_pci_aif_enable(), we allocate and assign new resources
to zdev without checking for or freeing prior state:

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

Could a guest or malicious userspace repeatedly invoke the ioctl to cause
unbounded memory exhaustion of pinned pages and airq_iv structures?

[Severity: High]
This is a pre-existing issue, but do the error paths in
kvm_s390_pci_aif_enable() accidentally return success?

If account_mem() or airq_iv_alloc_bit() fail, the code jumps to cleanup labels:

    if (account_mem(pcount))
        goto unpin2;
    ...
    if (bit == -1UL)
        goto unlock;

Since rc remains initialized to 0, does this erroneously return success to
userspace when enabling actually failed?

[Severity: High]
This is a pre-existing issue, but do the early error paths in
kvm_s390_pci_aif_enable() leak resources?

If pin_user_pages_fast(), account_mem(), or airq_iv_alloc_bit() fail, we
jump to the cleanup labels:

    unlock:
        mutex_unlock(&aift->aift_lock);
    unpin2:
        if (fib->fmt0.sum == 1)
            unpin_user_page(aisb_page);
    unpin1:
        unpin_user_page(aibv_page);
    out:
        return rc;

Does this miss calling kvm_s390_gisc_unregister() and unaccount_mem(),
leaving GISC registrations and locked_vm quotas permanently leaked?

[Severity: High]
This is a pre-existing issue, but do account_mem() and unaccount_mem() leak
struct user_struct reference counts?

Both functions increment the user refcount via get_uid():

    struct user_struct *user = get_uid(current_user());

However, free_uid() is never called in these functions. Could this lead to a
persistent memory leak and potential refcount overflow?

[Severity: High]
This is a pre-existing issue, but does unaccount_mem() risk quota corruption
by blindly using current_user() and current->mm?

When unaccount_mem() is called:

    struct user_struct *user = get_uid(current_user());
    if (user)
        atomic_long_sub(nr_pages, &user->locked_vm);

If this is invoked asynchronously during VFIO device closure (e.g., via fput
in a system workqueue), the current user will resolve to the worker thread.
Does this mean the original user's quota is never restored, and an unrelated
system worker's quota is incorrectly reduced?

[Severity: Critical]
This is a pre-existing issue, but does an unchecked return value from
airq_iv_create() risk a host kernel panic?

In kvm_s390_pci_aif_enable():

    zdev->aibv = airq_iv_create(msi_vecs, ...);

If airq_iv_create() returns NULL due to memory pressure, this is assigned to
zdev->aibv. Immediately after, kvm_zpci_set_airq() is called, which executes:

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

Does airq_iv_end() unconditionally dereference the potentially NULL pointer,
allowing unprivileged userspace or a guest to trigger a panic?

[Severity: Critical]
This is a pre-existing issue, but is the page pinning for the Adapter
Interrupt Bit Vector (AIBV) insufficient?

In kvm_s390_pci_aif_enable(), only a single page is pinned:

    npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);

The physical address passed to hardware contains the unvalidated offset:

    gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);

If a guest specifies an AIBV offset close to the end of the page, could the
hardware DMA engine write beyond the pinned page boundary and corrupt
adjacent host memory?

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