Re: [PATCH v3 1/4] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

[sashiko-bot@kernel.org]

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

New issues:
- [High] Repeated KVM_S390_ZPCIOP_REG_AEN ioctl calls overwrite kzdev->mm_account and kzdev->user_account without dropping previous references, permanently leaking mm_struct and user_struct.

Pre-existing issues:
- [High] kvm_s390_pci_aif_enable returns silent success (0) when memory accounting fails.
--

--- Patch [1]: [PATCH v3 1/4] KVM: s390: pci: Fix memory accounting for pinned/unpinned pages ---
commit 200dcce54ba1446ee99b11172f6ae91eeaacea27
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Fix memory accounting for pinned/unpinned pages

This commit changes the memory accounting functions to store the pinning
process's user_struct and mm_struct directly in the kvm_zdev structure.
This ensures that unaccount_mem() correctly decrements the references
from the original process context, rather than the context of the caller
performing the unpinning.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 720bb58cabe2e..dd17f8a7b4734 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c

[ ... ]

> @@ -190,33 +190,51 @@ static int kvm_zpci_clear_airq(struct zpci_dev *zdev)
> -static inline int account_mem(unsigned long nr_pages)
> +static inline int account_mem(struct kvm_zdev *kzdev, unsigned long nr_pages)
>  {
>  	struct user_struct *user = get_uid(current_user());
>  	unsigned long page_limit, cur_pages, new_pages;
> +	int rc = 0;

[ ... ]

> +	mmgrab(current->mm);
>  	atomic64_add(nr_pages, &current->mm->pinned_vm);
> +	kzdev->user_account = user;
> +	kzdev->mm_account = current->mm;

[Severity: High]
If userspace repeatedly calls the ioctl that triggers
kvm_s390_pci_aif_enable() for a device, can this overwrite
kzdev->mm_account and kzdev->user_account without dropping the previous
references?

Looking at kvm_s390_pci_aif_enable(), there doesn't appear to be a check to
see if the device is already registered before calling account_mem(). Calling
it multiple times would execute mmgrab() and get_uid() on each invocation,
unconditionally overwriting the pointers here and leaking the previous
mm_struct and user_struct.

>  
>  	return 0;
> +
> +out:
> +	free_uid(user);
> +	return rc;
>  }
>  
>  static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
> @@ -275,7 +293,7 @@ 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))
> +	if (account_mem(zdev->kzdev, pcount))
>  		goto unpin2;

[Severity: High]
This is a pre-existing issue, but does this error path return 0 (success) to
userspace when memory accounting fails?

In kvm_s390_pci_aif_enable(), the return code rc is initialized to 0. If
account_mem() fails and returns -ENOMEM, the code jumps to the unpin2
cleanup path without assigning the error code to rc.

This seems like it would mislead userspace into believing the interrupt
forwarding setup succeeded.

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