Re: [PATCH v1 4/5] 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 error path for `kvm_zpci_set_airq()` failure unregisters the wrong Interruption Subclass (ISC), leading to a reference count leak for the guest ISC and potential state corruption for the host ISC.

Pre-existing issues:
- [High] `kvm_s390_pci_aif_enable` lacks a check to prevent multiple AEN registrations for the same device, allowing a malicious userspace to trigger repeated allocations that leak pinned pages and `airq_iv` structures.
--

--- Patch [4]: [PATCH v1 4/5] KVM: s390: pci: Fix resource leak on IRQ registration failure ---
commit 3065114852ad54412b30b74a97b10b479bcd1a82
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 b3ffb8c255107..9a16a2eac65b8 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -334,11 +334,29 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	/* 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);

[Severity: High]
This is a pre-existing issue, but does kvm_s390_pci_aif_enable() lack a
check to prevent multiple AEN registrations for the same device?

If userspace repeatedly invokes the KVM_S390_ZPCIOP_REG_AEN ioctl on the
same device, could it blindly allocate new structures and overwrite tracking
fields like kzdev->aibv_pages and zdev->aibv?

kvm_s390_pci_aif_enable() {
    ...
    npinned = pin_user_pages_fast(hva, npages, FOLL_WRITE | FOLL_LONGTERM,
                                  kzdev->aibv_pages);
    ...
    zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
                                AIRQ_IV_BITLOCK |
                                AIRQ_IV_GUESTVEC,
                                phys_to_virt(fib->fmt0.aibv));
    ...
}

Would this irrevocably leak the previously pinned pages and airq_iv
structures?

>  
>  	/* 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;

[Severity: High]
Will this error path unregister the wrong Interruption Subclass (ISC)?

Since fib->fmt0.isc is overwritten with gisc earlier in
kvm_s390_pci_aif_enable():

    fib->fmt0.isc = gisc;
    ...
    rc = kvm_zpci_set_airq(zdev);

If kvm_zpci_set_airq() fails, the cleanup path eventually reaches out:

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

Could this cause the host to unregister the host NISC instead of the guest
ISC, potentially leaking the guest ISC's reference count and corrupting
state for the host ISC?

> +
> +	spin_lock_irq(&aift->gait_lock);
> +	gaite->count--;

[ ... ]

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