Re: [PATCH v3 4/4] KVM: s390: pci: Fix resource leak on IRQ registration failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The `aisbo` (Adapter Interrupt Summary Bit Offset) provided by userspace is not bounds-checked, leading to a host kernel memory out-of-bounds write.
- [Critical] The `aibv` address and `noi` (number of interrupts) provided by userspace are not bounds-checked, allowing hardware DMA to write out-of-bounds into unpinned host physical memory.
- [High] Missing check for already-enabled AIF allows massive resource leak, accounting exhaustion, and subsequent Use-After-Free on double registration.
--

--- Patch [4]: [PATCH v3 4/4] KVM: s390: pci: Fix resource leak on IRQ registration failure ---
commit 54ecbcfac4858add9c2dd09fc30b8b84a8fbfca3
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Fix resource leak on IRQ registration failure

Currently if kvm_zpci_set_airq() fails, kvm_s390_pci_aif_enable() returns
the error code but doesn't do any resource cleanup thus leaking resources.
Fix this by cleaning up all the resources such as the GAITE, AIBV, AISB and
unpinning any pinned pages.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 85f4fb5d36b38..d328bbd0dd997 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -337,19 +337,32 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,

[Severity: High]
This is a pre-existing issue, but does kvm_s390_pci_zpci_reg_aen() prevent
double registration?

If a malicious or buggy userspace repeatedly calls the KVM_S390_ZPCIOP_REG_AEN
ioctl on the same PCI device, kvm_s390_pci_aif_enable() unconditionally
allocates new resources:

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

Because it doesn't check if AIF is already enabled, it blindly pins new host
pages and overwrites the old zdev->aibv and zdev->aisb pointers.

Could this permanently leak the previous allocations and repeatedly increment
the memory accounting on user->locked_vm until it exhausts the limit?


[Severity: Critical]
This is a pre-existing issue, but does this code bounds-check the DMA length
against the pinned page?

kvm_s390_pci_aif_enable() calculates the physical base address for the
interrupt bit vector using a user-supplied page offset:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
    fib->fmt0.aibv = gaddr;
    ...
}

This physical address is passed directly to the hardware via
kvm_zpci_set_airq().

If the guest places the base address near the end of the page (e.g., offset
4095) and zdev->max_msi requires 2+ bytes, will the hardware DMA cross the
single pinned page's boundary?

Could this silently corrupt adjacent unpinned host physical pages?


[Severity: Critical]
This isn't a bug introduced by this patch, but is the aisbo value from
userspace properly bounds-checked?

In kvm_s390_pci_aif_enable(), gaite->aisbo is set directly from the
user-controlled fib->fmt0.aisbo:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    if (fib->fmt0.sum == 1) {
        gaite->aisbo = fib->fmt0.aisbo;
        gaite->aisb = virt_to_phys(page_address(aisb_page) +
                                   (fib->fmt0.aisb & ~PAGE_MASK));
    }
    ...
}

Upon an adapter interrupt, aen_host_forward() executes an out-of-bounds
bitwise write operation:

arch/s390/kvm/interrupt.c:aen_host_forward() {
    ...
    set_bit_inv(gaite->aisbo, phys_to_virt(gaite->aisb));
    ...
}

Since gaite->aisbo can be up to 255 (a 31-byte offset) and the guest offset
within the page can be up to 4095, the calculation (offset) + (aisbo / 8)
can reach 4126.

Could this cross the 4096-byte page boundary and perform a bitwise write up to
31 bytes out-of-bounds into the adjacent physical page?

>  	aift->kzdev[zdev->aisb] = zdev->kzdev;
>  	spin_unlock_irq(&aift->gait_lock);
>  
> -	/* Update guest FIB for re-issue */
> -	fib->fmt0.aisbo = zdev->aisb & 63;
> -	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
> -	fib->fmt0.isc = gisc;
> -
>  	/* Save some guest fib values in the host for later use */
> -	zdev->kzdev->fib.fmt0.isc = fib->fmt0.isc;
> +	zdev->kzdev->fib.fmt0.isc = gisc;
>  	zdev->kzdev->fib.fmt0.aibv = fib->fmt0.aibv;
> -	mutex_unlock(&aift->aift_lock);
>  
>  	/* Issue the clp to setup the irq now */
>  	rc = kvm_zpci_set_airq(zdev);
> -	return rc;
> +	if (!rc) {
> +		mutex_unlock(&aift->aift_lock);
> +		return rc;
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720175819.1723-1-alifm@linux.ibm.com?part=4