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

[sashiko-bot@kernel.org]

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