Re: [PATCH v5 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 5 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing bounds check for guest-provided Adapter Interrupt Bit Vector (AIBV) can lead to out-of-bounds DMA writes.
- [High] account_mem() and unaccount_mem() unconditionally leak a user_struct reference on every call.
- [High] The error path for kvm_zpci_set_airq() failure does not unwind allocated resources and permanently blocks future enablement.
- [High] Failure paths for memory accounting and vector allocation silently return success to userspace and leak process memory limits.
- [High] Missing NULL check on airq_iv_create() return value leads to kernel panic.
--

--- Patch [1]: [PATCH v5 1/6] KVM: s390: pci: Reject adapter interrupt forwarding if already enabled ---
commit 94b69f5ee2abdd1f23118fdfcfcf6fd754f160c4
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. This patch rejects any request to enable interrupt
forwarding if it is already enabled for the zPCI device, fixing resource
leaks when the ioctl is called multiple times.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 720bb58cabe2e..d2a11cdf6941d 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;
> +

[Severity: High]
This is a pre-existing issue, but does the error path for kvm_zpci_set_airq()
failure permanently block future enablement?

In kvm_s390_pci_aif_enable(), if kvm_zpci_set_airq(zdev) fails and returns
an error immediately, the state variables like zdev->kzdev->fib.fmt0.aibv
might be left non-zero, and pinned pages are not unpinned.

With the new check introduced in this patch, wouldn't any future attempts to
enable AIF hit this new aibv != 0 check and fail with -EINVAL?

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

Looking at account_mem():

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

It appears get_uid() increments the reference count, but neither account_mem()
nor unaccount_mem() ever call free_uid() on success or failure, potentially
leading to a refcount overflow and memory leak when KVM_S390_PCI_AIF_ENABLE is
called repeatedly. Is there a missing free_uid() call here?

[Severity: High]
This is a pre-existing issue, but do the failure paths for memory accounting
and vector allocation silently return success to userspace?

If account_mem() fails:

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    if (account_mem(pcount))
        goto unpin2;
    ...
}

Or 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;
    ...
}

It looks like the code jumps to unpin2 or unlock without setting rc to an
error code, returning 0. Could this exhaust process memory limits while
silently failing to set up AIF completely?

[Severity: High]
This is a pre-existing issue, but is there a missing NULL check on the return
value of airq_iv_create() that could lead to a kernel 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));
    ...
}

If airq_iv_create() returns NULL under memory pressure, it unconditionally
assigns NULL to zdev->aibv. Shortly after, kvm_zpci_set_airq() dereferences
this pointer:

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

Can this lead to a NULL pointer dereference?

[Severity: Critical]
This is a pre-existing issue, but is there a missing bounds check for the
guest-provided Adapter Interrupt Bit Vector (AIBV) that could lead to
out-of-bounds DMA writes?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
    ...
}

KVM explicitly pins only 1 page regardless of whether the requested bit vector
spans across a page boundary. If the guest provides a physical address very
close to the end of a page and the size of the bit vector requires more bytes,
could the hardware perform DMA writes spanning into the unpinned adjacent
page, potentially corrupting host memory?

>  	kvm = zdev->kzdev->kvm;
>  	msi_vecs = min_t(unsigned int, fib->fmt0.noi, zdev->max_msi);
>

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