Re: [PATCH v2 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 2 potential issue(s) to consider:

New issues:
- [High] The newly added error path for `kvm_zpci_set_airq()` failure uses the wrong ISC value when unregistering the guest ISC, leaking the guest ISC reference and potentially corrupting the host NISC reference count.

Pre-existing issues:
- [High] Missing state check allows multiple concurrent `KVM_S390_ZPCIOP_REG_AEN` ioctls, causing memory leaks of pinned pages, `mmgrab` references, and exhaustion of global `airq_iv` structures.
--

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

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

This commit adds cleanup code in kvm_s390_pci_aif_enable() to free the
associated adapter interrupt resources and unpin any pinned pages if
kvm_zpci_set_airq() fails. It does this by adding an error path that
resets the state and falls through to the existing cleanup labels.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 85f4fb5d36b38..f85d856b58c65 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -345,11 +345,29 @@ 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_aif_enable() lack a check
to verify if interrupt forwarding is already enabled?

If a userspace process repeatedly calls the registration ioctl on a single
KVM PCI device without deregistering first, the kernel appears to
unconditionally allocate new resources like pinned pages and airq_iv
structures.

Because the pointers to the previous allocations are overwritten without being
freed, could this allow a user to exhaust the host-wide global adapter
interrupt table bitmask (aift->sbv) and permanently leak pinned memory?

>  	/* Save some guest fib values in the host for later use */
>  	zdev->kzdev->fib.fmt0.isc = fib->fmt0.isc;
>  	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;
> +	}
> +
> +	/* Start cleanup */
> +	zdev->kzdev->fib.fmt0.isc = 0;
> +	zdev->kzdev->fib.fmt0.aibv = 0;
> +
> +	spin_lock_irq(&aift->gait_lock);
> +	gaite->count--;
> +	gaite->aisb = 0;
> +	gaite->gisc = 0;
> +	gaite->aisbo = 0;
> +	gaite->gisa = 0;
> +	aift->kzdev[zdev->aisb] = NULL;
> +	spin_unlock_irq(&aift->gait_lock);
> +
> +	airq_iv_release(zdev->aibv);
> +	zdev->aibv = NULL;

[Severity: High]
Does this new error path unregister the host ISC instead of the guest ISC?

Earlier in this function, the guest ISC in fib->fmt0.isc is replaced with the
host ISC:

    fib->fmt0.isc = gisc;

When kvm_zpci_set_airq() fails, the code now falls through to the out label:

out:
    kvm_s390_gisc_unregister(kvm, fib->fmt0.isc);

Since fib->fmt0.isc was overwritten, wouldn't this decrement the host ISC
reference count instead of the guest ISC reference count?

This could corrupt the host's alert mask and potentially disable interrupt
forwarding for other independent PCI devices sharing the host ISC, while
leaving the guest ISC reference permanently leaked.

>  
>  free_aisb:
>  	airq_iv_free_bit(aift->sbv, zdev->aisb);

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