Re: [PATCH v4 1/6] KVM: s390: pci: Reject adapter interrupt forwarding if already enabled

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing NULL check for the result of `airq_iv_create()` in `kvm_s390_pci_aif_enable`.
- [Critical] Missing page boundary check for guest-provided AIBV address allows an out-of-bounds write into host physical memory.
- [High] Reference count leak of `struct user_struct` in `account_mem` and `unaccount_mem`.
- [High] Memory un-accounting uses the current thread's credentials instead of the original ones, causing counter underflows.
- [High] Complete cleanup bypass when `kvm_zpci_set_airq()` fails in `kvm_s390_pci_aif_enable`.
- [High] Use-After-Free of `zdev` structure in `kvm_s390_pci_zpci_op()` due to lack of reference counting.
--

--- Patch [1]: [PATCH v4 1/6] KVM: s390: pci: Reject adapter interrupt forwarding if already enabled ---
commit 32372cae1fd4bf986fc6da1a214da1e85e7be6ad
Author: Farhan Ali <alifm@linux.ibm.com>
KVM: s390: pci: Reject adapter interrupt forwarding if already enabled

This commit prevents registering adapter interrupts via the MPCIFC instruction
if they are already registered. It avoids leaking resources by rejecting
requests to enable interrupt forwarding if it is already enabled for the zPCI
device.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
[ ... ]
>  	list_for_each_entry(kzdev, &kvm->arch.kzdev_list, entry) {
>  		if (kzdev->zdev->fh == fh) {
>  			zdev = kzdev->zdev;
>  			break;
>  		}
>  	}
>  	spin_unlock(&kvm->arch.kzdev_list_lock);
>  	return zdev;

[Severity: High]
This isn't a bug introduced by this patch, but is the returned zdev pointer
vulnerable to a use-after-free?

The pointer is retrieved and returned without elevating the device's reference
count via zpci_zdev_get(). If a concurrent hot-unplug occurs, the PCI core
might remove and free the device.

If the KVM ioctl thread then executes mutex_lock(&zdev->kzdev_lock) in
kvm_s390_pci_zpci_op() on the freed memory, can it lead to memory corruption
or a host crash?

[ ... ]
>  	struct user_struct *user = get_uid(current_user());
[ ... ]
>  	return 0;

[Severity: High]
This is a pre-existing issue, but does account_mem() leak the user_struct
reference?

The get_uid(current_user()) call fetches the user and increments its reference
count. However, the function returns 0 without ever pairing this with a
free_uid(user) call on the success path.

Could an unprivileged guest repeatedly enable and disable AIF to leak kernel
memory continuously and eventually overflow the reference counter?

[ ... ]
>  	struct user_struct *user = get_uid(current_user());
[ ... ]
>  	if (current->mm)
>  		atomic64_sub(nr_pages, &current->mm->pinned_vm);

[Severity: High]
This isn't a bug introduced by this patch, but does unaccount_mem()
incorrectly rely on the current thread's credentials?

When kvm_s390_pci_unregister_kvm() is invoked by the PCI core during device
hot-unplug or VFIO unbind, it executes in a system worker thread, not the
original KVM vcpu thread. It then calls unaccount_mem().

Does unaccount_mem() use current_user() and current->mm of the worker thread
instead of the original ones? Could this corrupt the memory accounting limits
of random processes, cause locked_vm to underflow, and permanently leak the
locked memory quota of the original KVM process?

[ ... ]
> @@ -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);
[ ... ]
>  	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
[ ... ]
>  	gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
>  	fib->fmt0.aibv = gaddr;

[Severity: Critical]
This is a pre-existing issue, but does this calculation allow an out-of-bounds
write into host physical memory?

Exactly one page is pinned, but the physical address is constructed by adding
an unaligned offset (fib->fmt0.aibv & ~PAGE_MASK). It doesn't appear to verify
if the required bit-vector size (msi_vecs bits) fits within the remaining
bounds of this single pinned page.

If a guest provides an address with a high intra-page offset, could the
hardware page-table walker or DMA overwrite the adjacent, unpinned physical
page and cause severe host memory corruption?

[ ... ]
>  	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);

[Severity: Critical]
This isn't a bug introduced by this patch, but does airq_iv_create() return a
pointer that is used without a NULL check?

If airq_iv_create() dynamically allocates memory and returns NULL on failure,
the pointer seems to be assigned to zdev->aibv and then dereferenced shortly
after in kvm_zpci_set_airq() via airq_iv_end().

Can an unprivileged guest trigger this path via KVM_S390_ZPCIOP_REG_AEN and
crash the host system if it is under memory pressure?

[ ... ]
>  	/* Issue the clp to setup the irq now */
>  	rc = kvm_zpci_set_airq(zdev);
>  	return rc;
>  
>  unlock:
>  	mutex_unlock(&aift->aift_lock);

[Severity: High]
This is a pre-existing issue, but does the function bypass the cleanup block
if kvm_zpci_set_airq() fails?

If the hardware rejects the configuration and kvm_zpci_set_airq() returns an
error, the function appears to return rc directly instead of jumping to the
cleanup labels (unlock:, unpin2:, out:).

Could this leave pinned pages, memory accounting, GAIT entries, and airq_iv
structures leaked? Since zdev->kzdev->fib.fmt0.aibv is left with a non-zero
value, does this persistently break the device for the guest?

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