[PATCH v1 0/5] KVM s390x PCI fixes

[PATCH v1 0/5] KVM s390x PCI fixes

[Farhan Ali]

Hi,

This series attempts to fixe the pre-existing issues[1] found by sashiko.

[1] https://lore.kernel.org/all/20260624063447.85DF51F000E9@smtp.kernel.org/


Farhan Ali (5):
  KVM: s390: pci: Fix refcount leak in memory accounting functions
  KVM: s390: pci: Fix missing error codes and memory unaccounting
  KVM: s390: pci: Fix NULL dereference on AIBV allocation failure
  KVM: s390: pci: Fix resource leak on IRQ registration failure
  KVM: s390: pci: Fix AIBV and AISB spanning multiple pages

 arch/s390/kvm/pci.c | 130 +++++++++++++++++++++++++++++++++++---------
 arch/s390/kvm/pci.h |   5 ++
 2 files changed, 109 insertions(+), 26 deletions(-)

-- 
2.43.0

[PATCH v1 1/5] KVM: s390: pci: Fix refcount leak in memory accounting functions

[Farhan Ali]

The account_mem() and unaccount_mem() functions call get_uid() which
increments the reference count of struct user_struct on every invocation.
But we don't decrement the count by calling free_uid().
Fix this by calling the free_uid() appropriately.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 720bb58cabe2..5bbbb1de4b5a 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -198,25 +198,32 @@ static inline void unaccount_mem(unsigned long nr_pages)
 		atomic_long_sub(nr_pages, &user->locked_vm);
 	if (current->mm)
 		atomic64_sub(nr_pages, &current->mm->pinned_vm);
+
+	free_uid(user);
 }
 
 static inline int account_mem(unsigned long nr_pages)
 {
 	struct user_struct *user = get_uid(current_user());
 	unsigned long page_limit, cur_pages, new_pages;
+	int rc = 0;
 
 	page_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
 
 	cur_pages = atomic_long_read(&user->locked_vm);
 	do {
 		new_pages = cur_pages + nr_pages;
-		if (new_pages > page_limit)
-			return -ENOMEM;
+		if (new_pages > page_limit) {
+			rc = -ENOMEM;
+			goto out;
+		}
 	} while (!atomic_long_try_cmpxchg(&user->locked_vm, &cur_pages, new_pages));
 
 	atomic64_add(nr_pages, &current->mm->pinned_vm);
 
-	return 0;
+out:
+	free_uid(user);
+	return rc;
 }
 
 static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
-- 
2.43.0

Re: [PATCH v1 1/5] KVM: s390: pci: Fix refcount leak in memory accounting functions

[sashiko-bot]

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

New issues:
- [High] Incorrect object lifetime management allows RLIMIT_MEMLOCK bypass.

Pre-existing issues:
- [High] Incorrect accounting context causes memory accounting leaks and potential local DoS via integer underflow.
- [High] Missing state check in `kvm_s390_pci_aif_enable()` allows malicious userspace to repeatedly register AIF, leaking memory and adapter interrupt bit vectors.
--

--- Patch [1]: [PATCH v1 1/5] KVM: s390: pci: Fix refcount leak in memory accounting functions ---
commit 81851b731e542e864a0b6023c4b3b8622937a3f3
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Fix refcount leak in memory accounting functions

This patch modifies account_mem() and unaccount_mem() to call free_uid()
after completing memory accounting operations. This is intended to fix a
reference count leak of the user_struct incremented by get_uid().

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 720bb58cabe2e..5bbbb1de4b5a0 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -198,25 +198,32 @@ static inline void unaccount_mem(unsigned long nr_pages)

[Severity: High]
This isn't a bug introduced by this patch, but does this logic query the
current execution context for unaccounting instead of storing and using the
original user and mm structures?

If unaccount_mem() is called from a worker thread or another daemon's context
where current->mm is NULL:

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

Wouldn't this leak the original pinned_vm charge?

Additionally, if current_user() resolves to the current thread's user (like
root), could this subtract pages from root's locked_vm and cause an integer
underflow, while the original user's quota is never restored?

>  		atomic_long_sub(nr_pages, &user->locked_vm);
>  	if (current->mm)
>  		atomic64_sub(nr_pages, &current->mm->pinned_vm);
> +
> +	free_uid(user);
>  }
>  
>  static inline int account_mem(unsigned long nr_pages)

[ ... ]

>  	do {
>  		new_pages = cur_pages + nr_pages;
> -		if (new_pages > page_limit)
> -			return -ENOMEM;
> +		if (new_pages > page_limit) {
> +			rc = -ENOMEM;
> +			goto out;
> +		}
>  	} while (!atomic_long_try_cmpxchg(&user->locked_vm, &cur_pages, new_pages));
>  
>  	atomic64_add(nr_pages, &current->mm->pinned_vm);
>  
> -	return 0;
> +out:
> +	free_uid(user);
> +	return rc;

[Severity: High]
Does dropping the user_struct reference immediately here fail to tie the
lifetime of the user's accounting object to the duration of the memory pin?

If a user registers AIF, passes the file descriptor to a persistent daemon,
and terminates their own processes, the user_struct reference count drops to
zero and is freed.

When the user logs back in, a fresh user_struct is allocated with
locked_vm = 0. Could this allow a malicious user to bypass RLIMIT_MEMLOCK
and pin unbounded amounts of memory?

>  }
>  
>  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 the AIF registration path lack a state
check to verify if AIF is already enabled before proceeding with memory
pinning and vector allocations?

If userspace calls KVM_S390_ZPCIOP_REG_AEN multiple times:

arch/s390/kvm/pci.c:kvm_s390_pci_zpci_reg_aen() {
    ...
    hostflag = !(args->u.reg_aen.flags & KVM_S390_ZPCIOP_REGAEN_HOST);
    return kvm_s390_pci_aif_enable(zdev, &fib, hostflag);
}

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    idx = srcu_read_lock(&kvm->srcu);
    hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aibv));
    ...
}

Will this blindly pin new pages, increment memory accounting, and allocate
new bit vectors, silently overwriting the old pointers in zdev?

Could this lead to a permanent leak of pinned host pages, airq_iv
allocations, and gaite entries?

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

[PATCH v1 2/5] KVM: s390: pci: Fix missing error codes and memory unaccounting

[Farhan Ali]

In kvm_s390_pci_aif_enable() two error paths failed to set error code,
causing the function to return 0 on failure. It also failed to rollback
memory accounting on failure. Fix both by propagating error code on
failure and calling unaccount_mem() in the cleanup path.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 5bbbb1de4b5a..42db20c2afca 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -282,14 +282,17 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	}
 
 	/* Account for pinned pages, roll back on failure */
-	if (account_mem(pcount))
+	rc = account_mem(pcount);
+	if (rc)
 		goto unpin2;
 
 	/* AISB must be allocated before we can fill in GAITE */
 	mutex_lock(&aift->aift_lock);
 	bit = airq_iv_alloc_bit(aift->sbv);
-	if (bit == -1UL)
+	if (bit == -1UL) {
+		rc = -ENOMEM;
 		goto unlock;
+	}
 	zdev->aisb = bit; /* store the summary bit number */
 	zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
 				    AIRQ_IV_BITLOCK |
@@ -333,6 +336,8 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	return rc;
 
 unlock:
+	if (pcount > 0)
+		unaccount_mem(pcount);
 	mutex_unlock(&aift->aift_lock);
 unpin2:
 	if (fib->fmt0.sum == 1)
-- 
2.43.0

Re: [PATCH v1 2/5] KVM: s390: pci: Fix missing error codes and memory unaccounting

[sashiko-bot]

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

Pre-existing issues:
- [High] `unaccount_mem()` incorrectly relies on `current` to reverse memory accounting, allowing arbitrary processes to corrupt their own memory limits.
- [High] Unrestricted overwrite of AIF structures on repeated ioctl calls leads to severe memory and resource leaks.
--

--- Patch [2]: [PATCH v1 2/5] KVM: s390: pci: Fix missing error codes and memory unaccounting ---
commit 38461cd36a2d54b4c7ee766e7c866405617494d9
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Fix missing error codes and memory unaccounting

In kvm_s390_pci_aif_enable(), two error paths failed to set an error code
and did not roll back memory accounting on failure. This commit propagates
the error code and calls unaccount_mem() in the cleanup path.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5bbbb1de4b5a0..42db20c2afca5 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -282,14 +282,17 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	}
>  
>  	/* Account for pinned pages, roll back on failure */
> -	if (account_mem(pcount))
> +	rc = account_mem(pcount);
> +	if (rc)
>  		goto unpin2;
>  
>  	/* AISB must be allocated before we can fill in GAITE */
>  	mutex_lock(&aift->aift_lock);
>  	bit = airq_iv_alloc_bit(aift->sbv);
> -	if (bit == -1UL)
> +	if (bit == -1UL) {
> +		rc = -ENOMEM;
>  		goto unlock;
> +	}
>  	zdev->aisb = bit; /* store the summary bit number */
>  	zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
>  				    AIRQ_IV_BITLOCK |

[Severity: High]
This is a pre-existing issue, but could this unconditional overwrite of
zdev->aibv lead to a memory and resource leak?

If a userspace caller issues the KVM_S390_ZPCIOP_REG_AEN ioctl multiple times
for the same device, kvm_s390_pci_aif_enable() lacks state validation (e.g.,
verifying zdev->kzdev->fib.fmt0.aibv == 0).

Would this cause the code to repeatedly pin new pages, allocate new airq_iv
structures, and blindly overwrite the zdev->aibv and zdev->aisb pointers,
permanently orphaning the previously allocated resources?

> @@ -333,6 +336,8 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	return rc;
>  
>  unlock:
> +	if (pcount > 0)
> +		unaccount_mem(pcount);

[Severity: High]
This isn't a bug introduced by this patch, but does unaccount_mem()
incorrectly rely on current to reverse memory accounting?

If process A pins memory via ioctl (accounting against process A's limits) and
passes the KVM file descriptor to process B, and process B closes it, the
file's .release handler triggers KVM unregistration.

Calling unaccount_mem() in process B's context would subtract from process B's
limits while leaking process A's limits.

Could this allow an unprivileged process to artificially underflow its
locked_vm and pinned_vm limits, bypassing RLIMIT_MEMLOCK and allowing
unlimited memory pinning?

>  	mutex_unlock(&aift->aift_lock);
>  unpin2:
>  	if (fib->fmt0.sum == 1)

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

[PATCH v1 3/5] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

[Farhan Ali]

The airq_iv_create() can return NULL on failure, but the return value was
never checked. If it fails, zdev->aibv will be NULL and fail when
derefenced in kvm_zpci_set_airq(). Add a NULL check and free the previously
allocated AISB bit and zdev->aisb on failure.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 42db20c2afca..b3ffb8c25510 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -299,6 +299,11 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 				    AIRQ_IV_GUESTVEC,
 				    phys_to_virt(fib->fmt0.aibv));
 
+	if (!zdev->aibv) {
+		rc = -ENOMEM;
+		goto free_aisb;
+	}
+
 	spin_lock_irq(&aift->gait_lock);
 	gaite = aift->gait + zdev->aisb;
 
@@ -335,6 +340,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	rc = kvm_zpci_set_airq(zdev);
 	return rc;
 
+free_aisb:
+	airq_iv_free_bit(aift->sbv, zdev->aisb);
+	zdev->aisb = 0;
 unlock:
 	if (pcount > 0)
 		unaccount_mem(pcount);
-- 
2.43.0

Re: [PATCH v1 3/5] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Hardware DMA out-of-bounds write due to missing page boundary validation for the AIBV.
- [High] Unbounded resource leak and potential Use-After-Free on repeated KVM_S390_ZPCIOP_REG_AEN calls.
- [High] Context mismatch in memory accounting leads to leakage or underflow during asynchronous teardown.
--

--- Patch [3]: [PATCH v1 3/5] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure ---
commit efdcaa0691c30263d9d4c19ea7c5478853d5a3ab
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

The commit adds a NULL check for the return value of airq_iv_create() to
prevent a potential NULL pointer dereference. If the allocation fails, it
now properly frees the previously allocated AISB bit before returning an
error.

> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -299,6 +299,11 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,

[Severity: Critical]
This isn't a regression introduced by this patch, but is there a missing page
boundary validation for the AIBV that could lead to an out-of-bounds DMA
write?

If the guest provides an unaligned guest physical address for the AIBV that
crosses a page boundary, does the pinning logic fail to account for vector
lengths spanning across boundaries?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
    ...
}

Could the hardware firmware sequentially DMA interrupt data across the page
boundary into an unpinned host physical page?

>  				    AIRQ_IV_GUESTVEC,
>  				    phys_to_virt(fib->fmt0.aibv));
>  
> +	if (!zdev->aibv) {
> +		rc = -ENOMEM;
> +		goto free_aisb;
> +	}
> +

[Severity: High]
This isn't a regression introduced by this patch, but does this path leak
the airq_iv bit vectors and cause a potential use-after-free?

If a user-space process repeatedly triggers the KVM_S390_ZPCIOP_REG_AEN ioctl,
does this code blindly overwrite zdev->aisb and zdev->aibv without clearing
the global aift->kzdev[old_aisb] reference?

If the device is later deregistered, only the newest AISB index would be
cleared. If a hardware interrupt triggers for the orphaned AISB bit, could
aen_host_forward() access the freed kzdev structure?

>  	spin_lock_irq(&aift->gait_lock);
>  	gaite = aift->gait + zdev->aisb;
>  
> @@ -335,6 +340,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	rc = kvm_zpci_set_airq(zdev);
>  	return rc;
>  
> +free_aisb:
> +	airq_iv_free_bit(aift->sbv, zdev->aisb);
> +	zdev->aisb = 0;
>  unlock:
>  	if (pcount > 0)
>  		unaccount_mem(pcount);

[Severity: High]
This isn't a regression introduced by this patch, but does unaccount_mem()
incorrectly rely on current->mm in asynchronous teardown contexts?

If unaccount_mem() is called asynchronously (for example, during a VFIO
device removal or sysfs unbind):

arch/s390/kvm/pci.c:unaccount_mem() {
    struct user_struct *user = get_uid(current_user());
    ...
    if (current->mm)
        atomic64_sub(nr_pages, &current->mm->pinned_vm);
    ...
}

Would current point to the worker thread rather than the QEMU process that
originally pinned the memory, leading to an underflow of the wrong memory
quotas and leaking the original quotas?

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

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

[Farhan Ali]

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.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index b3ffb8c25510..9a16a2eac65b 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);
 
 	/* 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;
 
 free_aisb:
 	airq_iv_free_bit(aift->sbv, zdev->aisb);
-- 
2.43.0

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

[sashiko-bot]

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

[PATCH v1 5/5] KVM: s390: pci: Fix AIBV and AISB spanning multiple pages

[Farhan Ali]

The AIBV (adapter interrupt bit vector) holds one bit per MSI-X vector for
a given function. With a maximum of 2048 MSI-X vectors per function, the
AIBV can be up to 256 bytes. Because a guest is free to place the AIBV at
any alignment, it may cross a page boundary. Pin all pages covered by the
AIBV to handle this correctly.

The AISB (adapter interrupt summary bit) holds one bit per zPCI function.
Its total size is configuration-dependent and not architecturally bounded,
so the AISB offset is used to detect whether it crosses a page boundary
and pin accordingly.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/kvm/pci.c | 78 ++++++++++++++++++++++++++++++++++-----------
 arch/s390/kvm/pci.h |  5 +++
 2 files changed, 64 insertions(+), 19 deletions(-)

diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
index 9a16a2eac65b..5fe07181f3d6 100644
--- a/arch/s390/kvm/pci.c
+++ b/arch/s390/kvm/pci.c
@@ -229,13 +229,15 @@ static inline int account_mem(unsigned long nr_pages)
 static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 				   bool assist)
 {
-	struct page *pages[1], *aibv_page, *aisb_page = NULL;
-	unsigned int msi_vecs, idx;
+	struct page *aibv_page, *aisb_page = NULL;
+	int gisc, npages, npinned, pcount = 0;
+	unsigned int msi_vecs, idx, size;
 	struct zpci_gaite *gaite;
 	unsigned long hva, bit;
+	struct kvm_zdev *kzdev;
 	struct kvm *kvm;
 	phys_addr_t gaddr;
-	int rc = 0, gisc, npages, pcount = 0;
+	int rc = 0;
 
 	/*
 	 * Interrupt forwarding is only applicable if the device is already
@@ -244,6 +246,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 	if (zdev->gisa == 0)
 		return -EINVAL;
 
+	kzdev = zdev->kzdev;
 	kvm = zdev->kzdev->kvm;
 	msi_vecs = min_t(unsigned int, fib->fmt0.noi, zdev->max_msi);
 
@@ -253,32 +256,63 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 		return gisc;
 
 	/* Replace AIBV address */
+	size = BITS_TO_LONGS(msi_vecs) * sizeof(unsigned long);
+	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);
+	if (npages > MAX_AIF_PAGES) {
+		rc = -EINVAL;
+		goto out;
+	}
+
 	idx = srcu_read_lock(&kvm->srcu);
 	hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aibv));
-	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
+	npinned = pin_user_pages_fast(hva, npages, FOLL_WRITE | FOLL_LONGTERM,
+				      kzdev->aibv_pages);
 	srcu_read_unlock(&kvm->srcu, idx);
-	if (npages < 1) {
+	if (npinned <= 0) {
 		rc = -EIO;
 		goto out;
 	}
-	aibv_page = pages[0];
-	pcount++;
+
+	kzdev->aibv_npages = npinned;
+	if (npinned < npages) {
+		rc = -EIO;
+		goto unpin1;
+	}
+
+	pcount += npinned;
+	aibv_page = kzdev->aibv_pages[0];
 	gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
 	fib->fmt0.aibv = gaddr;
 
 	/* Pin the guest AISB if one was specified */
 	if (fib->fmt0.sum == 1) {
+		size = (fib->fmt0.aisbo / 8) + 1;
+		npages = DIV_ROUND_UP((fib->fmt0.aisb & ~PAGE_MASK) + size,
+				      PAGE_SIZE);
+
+		if (npages > MAX_AIF_PAGES) {
+			rc = -EINVAL;
+			goto unpin1;
+		}
+
 		idx = srcu_read_lock(&kvm->srcu);
 		hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aisb));
-		npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM,
-					     pages);
+		npinned = pin_user_pages_fast(hva, npages,
+					     FOLL_WRITE | FOLL_LONGTERM,
+					     kzdev->aisb_pages);
 		srcu_read_unlock(&kvm->srcu, idx);
-		if (npages < 1) {
+		if (npinned <= 0) {
 			rc = -EIO;
 			goto unpin1;
 		}
-		aisb_page = pages[0];
-		pcount++;
+
+		kzdev->aisb_npages = npinned;
+		if (npinned < npages) {
+			rc = -EIO;
+			goto unpin2;
+		}
+		aisb_page = kzdev->aisb_pages[0];
+		pcount += npinned;
 	}
 
 	/* Account for pinned pages, roll back on failure */
@@ -366,10 +400,14 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
 		unaccount_mem(pcount);
 	mutex_unlock(&aift->aift_lock);
 unpin2:
-	if (fib->fmt0.sum == 1)
-		unpin_user_page(aisb_page);
+	if (fib->fmt0.sum == 1) {
+		unpin_user_pages(kzdev->aisb_pages,
+				 kzdev->aisb_npages);
+		kzdev->aisb_npages = 0;
+	}
 unpin1:
-	unpin_user_page(aibv_page);
+	unpin_user_pages(kzdev->aibv_pages, kzdev->aibv_npages);
+	kzdev->aibv_npages = 0;
 out:
 	kvm_s390_gisc_unregister(kvm, fib->fmt0.isc);
 	return rc;
@@ -426,12 +464,14 @@ static int kvm_s390_pci_aif_disable(struct zpci_dev *zdev, bool force)
 	kzdev->fib.fmt0.aibv = 0;
 
 	if (vpage) {
-		unpin_user_page(vpage);
-		pcount++;
+		unpin_user_pages(kzdev->aibv_pages, kzdev->aibv_npages);
+		pcount += kzdev->aibv_npages;
+		kzdev->aibv_npages = 0;
 	}
 	if (spage) {
-		unpin_user_page(spage);
-		pcount++;
+		unpin_user_pages(kzdev->aisb_pages, kzdev->aisb_npages);
+		pcount += kzdev->aisb_npages;
+		kzdev->aisb_npages = 0;
 	}
 	if (pcount > 0)
 		unaccount_mem(pcount);
diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h
index ff0972dd5e71..a2d550f06b6c 100644
--- a/arch/s390/kvm/pci.h
+++ b/arch/s390/kvm/pci.h
@@ -17,10 +17,15 @@
 #include <asm/airq.h>
 #include <asm/cpu.h>
 
+#define MAX_AIF_PAGES 2
 struct kvm_zdev {
 	struct zpci_dev *zdev;
 	struct kvm *kvm;
 	struct zpci_fib fib;
+	struct page *aibv_pages[MAX_AIF_PAGES];
+	int aibv_npages;
+	struct page *aisb_pages[MAX_AIF_PAGES];
+	int aisb_npages;
 	struct list_head entry;
 };
 
-- 
2.43.0

Re: [PATCH v1 5/5] KVM: s390: pci: Fix AIBV and AISB spanning multiple pages

[sashiko-bot]

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

New issues:
- [Medium] Multi-page pinning relies on a single host virtual address (HVA) translation, which fails across KVM memslot boundaries.

Pre-existing issues:
- [Critical] Hardware DMA and kernel virtual accesses overflow into unrelated physical pages due to lack of physical contiguity.
- [High] Repeated registration ioctls leak pinned memory pages and `airq_iv` structures.
--

--- Patch [5]: [PATCH v1 5/5] KVM: s390: pci: Fix AIBV and AISB spanning multiple pages ---
commit 4f8b3d86d13ac2b1f9a5979bfc8a008742cfc9de
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Fix AIBV and AISB spanning multiple pages

This patch modifies KVM s390 pci interrupt forwarding to pin multiple pages
for AIBV and AISB structures if they cross a page boundary. It calculates the
required number of pages and pins them to handle unbounded sizes or varying
alignments correctly.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 9a16a2eac65b8..5fe07181f3d6a 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -229,13 +229,15 @@ static inline int account_mem(unsigned long nr_pages)
>  static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  				   bool assist)
>  {

[ ... ]

>  	/* Replace AIBV address */
> +	size = BITS_TO_LONGS(msi_vecs) * sizeof(unsigned long);
> +	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);
> +	if (npages > MAX_AIF_PAGES) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
>  	idx = srcu_read_lock(&kvm->srcu);
>  	hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aibv));
> -	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
> +	npinned = pin_user_pages_fast(hva, npages, FOLL_WRITE | FOLL_LONGTERM,
> +				      kzdev->aibv_pages);

[Severity: Medium]
If the AIBV crosses a page boundary that aligns with a KVM memslot boundary
where the adjacent memslot maps to a disjoint HVA region, won't this pin the
wrong host memory pages since we only translate the first GFN to an HVA?

[Severity: High]
This is a pre-existing issue, but if a guest repeatedly invokes the
registration ioctl for the same zPCI device, won't this leak pinned memory
pages by overwriting kzdev->aibv_pages without releasing the previous pages?

Similarly, when zdev->aibv is overwritten later in 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));

Does this leak the previous airq_iv structure and exhaust adapter interrupt
bit vectors?

>  	srcu_read_unlock(&kvm->srcu, idx);
> -	if (npages < 1) {
> +	if (npinned <= 0) {
>  		rc = -EIO;
>  		goto out;
>  	}
> -	aibv_page = pages[0];
> -	pcount++;
> +
> +	kzdev->aibv_npages = npinned;
> +	if (npinned < npages) {
> +		rc = -EIO;
> +		goto unpin1;
> +	}
> +
> +	pcount += npinned;
> +	aibv_page = kzdev->aibv_pages[0];
>  	gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
>  	fib->fmt0.aibv = gaddr;

[Severity: Critical]
This is a pre-existing issue, but if npinned is greater than 1 and the user
pages are not physically contiguous, won't providing a single base physical
address (gaddr) cause hardware DMA accesses to linearly overflow into
unrelated physical memory adjacent to aibv_page?

Also, later in 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));

Does phys_to_virt(fib->fmt0.aibv) similarly cause kernel virtual accesses
to corrupt unrelated physical memory if the pages are not contiguous?

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