Re: [GIT PULL 1/7] KVM: s390: pci: Reject adapter interrupt forwarding if already enabled

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] kvm_s390_pci_aif_enable() silently returns success (0) when account_mem() or airq_iv_alloc_bit() fail, leaving the device non-functional in the guest.
- [High] Missing NULL check for airq_iv_create() return value causes a guaranteed kernel panic on allocation failure.
- [High] The helper functions account_mem() and unaccount_mem() permanently leak user_struct reference counts and inaccurately uncharge teardown threads.
--

--- Patch [1]: [GIT PULL 1/7] KVM: s390: pci: Reject adapter interrupt forwarding if already enabled ---
commit 834c3470cb709cfa95cf6f7b8315fe450f54610b
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Reject adapter interrupt forwarding if already enabled
    
The MPCIFC instruction doesn't allow registering adapter interrupts without
first unregistering. So reject any request to enable interrupt forwarding
if its already enabled for the zPCI device. This also fixes overwriting and
thus leaking resources when the ioctl is called multiple times for the same
device.

Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Cc: stable@vger.kernel.org

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5b075c38998e3..02ec50a01fed2 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -237,6 +237,10 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	if (zdev->gisa == 0)
>  		return -EINVAL;
>  
> +	/* AIF already enabled for the device */
> +	if (zdev->kzdev->fib.fmt0.aibv != 0)
> +		return -EINVAL;
> +
>  	kvm = zdev->kzdev->kvm;
>  	msi_vecs = min_t(unsigned int, fib->fmt0.noi, zdev->max_msi);
>  

[Severity: High]
This is a pre-existing issue, but does kvm_s390_pci_aif_enable() silently
return success when account_mem() fails?

If account_mem() fails, it jumps to unpin2, but rc remains 0:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	/* Account for pinned pages, roll back on failure */
	if (account_mem(pcount))
		goto unpin2;
    ...
unpin2:
	if (fib->fmt0.sum == 1)
		unpin_user_page(aisb_page);
unpin1:
	unpin_user_page(aibv_page);
out:
	return rc;
}

The same happens if airq_iv_alloc_bit() fails:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	bit = airq_iv_alloc_bit(aift->sbv);
	if (bit == -1UL)
		goto unlock;
    ...
}

Should rc be updated to a negative error code before jumping to these cleanup
labels?


[Severity: High]
This is a pre-existing issue, but could airq_iv_create() return NULL and cause
a panic?

arch/s390/kvm/pci.c: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));

	spin_lock_irq(&aift->gait_lock);
    ...
}

If airq_iv_create() fails and returns NULL, zdev->aibv becomes NULL. Later,
when kvm_zpci_set_airq() is called, it might dereference this NULL pointer:

arch/s390/kvm/pci.c:kvm_zpci_set_airq() {
    ...
	fib.fmt0.noi = airq_iv_end(zdev->aibv);
    ...
}

Should we check the return value of airq_iv_create() and handle the failure?


[Severity: High]
This isn't a bug introduced by this patch, but do the accounting functions leak
user_struct references?

In account_mem():

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

And in unaccount_mem():

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

	if (user)
		atomic_long_sub(nr_pages, &user->locked_vm);
	if (current->mm)
		atomic64_sub(nr_pages, &current->mm->pinned_vm);
    ...
}

Both functions call get_uid(current_user()) which increments the user's
reference count, but they do not seem to call free_uid().

Additionally, unaccount_mem() might be called in asynchronous teardown
contexts where current points to a teardown worker thread instead of the
original VM process. Does this lead to inaccurate uncharging of the
teardown thread?

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