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

[sashiko-bot@kernel.org]

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