Re: [PATCH] KVM: s390: pci: Fix handling of AIF enable without AISB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing state check allows repeated `KVM_S390_ZPCIOP_REG_AEN` ioctls to leak resources and create dangling pointers.
- [Critical] Unvalidated guest-supplied offsets for the Adapter Interrupt Bit Vector (AIBV) can cause the host firmware to write beyond the pinned page boundary.
- [High] Error paths for `account_mem` and `airq_iv_alloc_bit` do not set an error code, leading to silent failures, and bypass memory accounting rollback.
- [High] Unchecked return value of `airq_iv_create()` leads to a kernel NULL pointer dereference on memory exhaustion.
- [High] The memory accounting functions `account_mem()` and `unaccount_mem()` leak a reference count to the `user_struct` on every call.
- [High] Failure in hardware configuration via `kvm_zpci_set_airq` bypasses local cleanup labels, leaving resources allocated.
--

--- Patch [1]: [PATCH] KVM: s390: pci: Fix handling of AIF enable without AISB ---
commit 770cb99be23b8c546be19f495246867591501586
Author: Matthew Rosato <mjrosato@linux.ibm.com>

KVM: s390: pci: Fix handling of AIF enable without AISB
    
When a guest seeks to register IRQs without a summary bit specified,
ensure that the associated GAITE then stores 0 for the guest AISB
location instead of virt_to_phys(page_address(NULL)).
    
Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
Cc: stable@vger.kernel.org
Reviewed-by: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 686113be05309..720bb58cabe2e 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -300,9 +300,14 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  
>  	gaite->gisc = fib->fmt0.isc;
>  	gaite->count++;
> -	gaite->aisbo = fib->fmt0.aisbo;
> -	gaite->aisb = virt_to_phys(page_address(aisb_page) + (fib->fmt0.aisb &
> -							      ~PAGE_MASK));
> +	if (fib->fmt0.sum == 1) {
> +		gaite->aisbo = fib->fmt0.aisbo;
> +		gaite->aisb = virt_to_phys(page_address(aisb_page) +
> +					   (fib->fmt0.aisb & ~PAGE_MASK));
> +	} else {
> +		gaite->aisbo = 0;
> +		gaite->aisb = 0;
> +	}
>  	aift->kzdev[zdev->aisb] = zdev->kzdev;
>  	spin_unlock_irq(&aift->gait_lock);
>  

[Severity: High]
This is a pre-existing issue, but does this error path properly set an error
code and roll back accounting?

In kvm_s390_pci_aif_enable():

    if (account_mem(pcount))
        goto unpin2;

    /* AISB must be allocated before we can fill in GAITE */
    mutex_lock(&aift->aift_lock);
    bit = airq_iv_alloc_bit(aift->sbv);
    if (bit == -1UL)
        goto unlock;

If account_mem() fails, rc appears to remain 0.

If airq_iv_alloc_bit() fails, the code jumps to unlock and falls through to
unpin2. This successfully unpins the pages but bypasses unaccount_mem().

Does this permanently leak the user's memory limits?

[Severity: High]
This isn't a bug introduced by this patch, but does the return value of
airq_iv_create() need validation here?

In 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, zdev->aibv is
assigned NULL.

Immediately after, kvm_zpci_set_airq() is called, which appears to
unconditionally dereference it:

kvm_zpci_set_airq() {
    ...
    fib.fmt0.noi = airq_iv_end(zdev->aibv);
    ...

Could this lead to a NULL pointer dereference?

[Severity: Critical]
This is a pre-existing issue, but does the ioctl handler check if AEN is
already enabled before allowing another registration?

Looking at kvm_s390_pci_zpci_op():

    r = kvm_s390_pci_zpci_reg_aen(zdev, args);

If a guest repeatedly calls the KVM_S390_ZPCIOP_REG_AEN ioctl, it
unconditionally invokes kvm_s390_pci_aif_enable().

Inside kvm_s390_pci_aif_enable():

    zdev->aisb = bit;
    zdev->aibv = airq_iv_create(...);

Is it possible this overwrites previous bit indexes and pointers without
cleaning up the old resources? This looks like it might leave dangling
pointers in the global aift->kzdev array for the overwritten bits.

[Severity: Critical]
This isn't a bug introduced by this patch, but does this code restrict the
guest-provided offset within the single pinned page?

In kvm_s390_pci_aif_enable():

    npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
    ...
    gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);

Since only 1 page is pinned, if the guest offset plus the required interrupt
vector size exceeds the page boundary, can the hardware adapter end up
writing interrupt bits into unpinned host memory?

[Severity: High]
This is a pre-existing issue, but do these accounting functions leak a
reference count?

In account_mem():

    struct user_struct *user = get_uid(current_user());

And in unaccount_mem():

    struct user_struct *user = get_uid(current_user());

Both functions call get_uid(), which increments the reference count, but
they do not appear to call free_uid() to release it.

Additionally, unaccount_mem() uses current_user(), which might not be the
same user who originally accounted the memory if AEN is disabled by a
different process. Are we at risk of mismatched locked_vm accounting?

[Severity: High]
This isn't a bug introduced by this patch, but does an error from
kvm_zpci_set_airq() bypass the cleanup paths?

In kvm_s390_pci_aif_enable():

    /* Issue the clp to setup the irq now */
    rc = kvm_zpci_set_airq(zdev);
    return rc;

unlock:
    mutex_unlock(&aift->aift_lock);
unpin2:
...

If kvm_zpci_set_airq() fails, the function returns directly. Should the
function jump to the cleanup labels instead to prevent leaving pages pinned
and structures like airq_iv permanently allocated?

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