Re: [GIT PULL 5/5] KVM: s390: pci: Fix handling of AIF enable without AISB

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing page boundary check allows unprivileged guest to trigger out-of-bounds hardware DMA writes.
- [High] Missing error code assignments cause silent failures and memory accounting leaks.
- [High] Missing NULL check on the return value of airq_iv_create() leads to an unconditional host kernel panic.
- [High] Repeatedly enabling AIF on an already-registered device causes state corruption and massive resource leaks.
- [High] Kernel resources are not rolled back if the PCI hardware configuration (kvm_zpci_set_airq) fails.
- [High] account_mem() and unaccount_mem() perpetually leak user_struct reference counts.
- [High] The guest ISC reference count leaks on error paths, leading to a 32-bit integer overflow and Denial of Service.
--

--- Patch [1]: [GIT PULL 5/5] KVM: s390: pci: Fix handling of AIF enable without AISB ---
commit d999e1412ad1f5b8348f25eacbb5dfb9be451bb1
Author: Matthew Rosato <mjrosato@linux.ibm.com>

KVM: s390: pci: Fix handling of AIF enable without AISB
    
This commit modifies the KVM s390 PCI interrupt forwarding (AIF) enablement. 
It ensures that when a guest registers IRQs without a summary bit (AISB), the 
guest AISB location is explicitly set to 0 rather than a potentially invalid 
physical address derived from a NULL pointer.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5b075c38998e3..dc748cde4703b 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: Critical]
This is a pre-existing issue, but does this lack of boundary checking allow
out-of-bounds hardware DMA writes?

Looking at kvm_s390_pci_aif_enable(), the code pins exactly one page for the
guest's Adapter Interruption Broadcast Vector (AIBV):

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

If the guest supplies an offset near the end of the page and a large number
of MSI vectors, could the hardware DMA overflow the single pinned page and
corrupt adjacent unpinned physical memory?

[Severity: High]
This is a pre-existing issue, but does this error path leak the guest ISC
reference count?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	gisc = kvm_s390_gisc_register(kvm, fib->fmt0.isc);
	if (gisc < 0)
		return gisc;
    ...
	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
	srcu_read_unlock(&kvm->srcu, idx);
	if (npages < 1) {
		rc = -EIO;
		goto out;
	}
    ...
out:
	return rc;
}

If pin_user_pages_fast() fails, the function branches to the out label but
appears to omit calling kvm_s390_gisc_unregister(). Could this lead to a
32-bit integer overflow on gi->alert.ref_count[gisc]?

[Severity: High]
This is a pre-existing issue, but do these error paths silently fail and leak
memory accounting limits?

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

If account_mem() or airq_iv_alloc_bit() fail, the function jumps to cleanup
labels but returns rc, which is initialized to 0. 

Additionally, in the airq_iv_alloc_bit() error path, unaccount_mem() appears
to be missing entirely. Does this silently leak the locked_vm memory
accounting?

[Severity: High]
This is a pre-existing issue, but what prevents a guest from repeatedly calling
KVM_S390_ZPCIOP_REG_AEN for the same device?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	zdev->aisb = bit; /* store the summary bit number */
	zdev->aibv = airq_iv_create(msi_vecs, AIRQ_IV_DATA |
    ...
}

If userspace calls KVM_S390_ZPCIOP_REG_AEN multiple times for the same PCI
device, the function appears to lack a check verifying if zdev->aibv is
already set. 

Could this blindly allocate and pin new resources while unconditionally
overwriting the existing pointers, permanently leaking the previously
pinned pages and airq_iv structures?

[Severity: High]
This is a pre-existing issue, but does this code risk a NULL pointer
dereference if airq_iv_create() fails?

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 in out-of-memory conditions, the result is
assigned to zdev->aibv without validation. 

Could this lead to an unconditional host kernel panic when
kvm_zpci_set_airq() subsequently dereferences it via airq_iv_end(zdev->aibv)?

[Severity: High]
This is a pre-existing issue, but are kernel resources permanently leaked if
the hardware configuration fails?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	/* Issue the clp to setup the irq now */
	rc = kvm_zpci_set_airq(zdev);
	return rc;

unlock:
    ...
}

If kvm_zpci_set_airq() returns an error because the hardware rejects the CLP
instruction, the function returns directly. Should it use the cleanup labels
to avoid leaking pinned guest memory pages and GISC registrations?

[Severity: High]
This is a pre-existing issue, but do account_mem() and unaccount_mem()
perpetually leak user_struct references?

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

Both functions call get_uid(current_user()), which increments the reference
count. However, neither function appears to ever call free_uid() to release
the acquired reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727095359.107671-6-borntraeger@de.ibm.com?part=1