Re: [GIT PULL 6/7] KVM: s390: pci: Validate AIBV and AISB before pinning guest pages

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing NULL check for `aisb_page` leads to host memory corruption via a garbage physical address in `gaite->aisb`.
- [Critical] Missing validation of the user-provided `aisbo` allows an out-of-bounds bit flip in host physical memory.
- [High] Missing software state rollback if hardware configuration fails in `kvm_zpci_set_airq()`.
- [High] Missing `kvm_s390_gisc_unregister()` on error paths leaks GISC reference counts.
- [High] No check for already enabled AEN allows guests to leak host resources via repeated ioctl calls.
- [High] Failures in `account_mem()` and `airq_iv_alloc_bit()` return a false success to userspace and leak memory accounting.
- [High] Missing NULL check for `airq_iv_create()` return value leads to host kernel panic.
--

--- Patch [1]: [GIT PULL 6/7] KVM: s390: pci: Validate AIBV and AISB before pinning guest pages ---
commit b6f9e911464048e2480c6dd2f890285a31bef43d
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Validate AIBV and AISB before pinning guest pages

This commit adds validation checks for the AIBV and AISB addresses provided
by the guest during PCI interrupt forwarding setup. It ensures that the AIBV
does not span more than one page and that the AISB is doubleword aligned.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 5b075c38998e3..efeee49e18ea3 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -246,6 +246,14 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  		return gisc;
>  
>  	/* Replace AIBV address */
> +	size = BITS_TO_LONGS(msi_vecs + fib->fmt0.aibvo) * sizeof(unsigned long);
> +	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);
> +	/* AIBV cannot span more than 1 page */
> +	if (npages > 1) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the new error path leak
the GISC reference count?

If npages > 1 triggers, the code jumps to out without calling
kvm_s390_gisc_unregister(), which was registered earlier:

	gisc = kvm_s390_gisc_register(kvm, fib->fmt0.isc);
	if (gisc < 0)
		return gisc;

Can a guest repeatedly pass spanning addresses to overflow the reference counter?

>  	idx = srcu_read_lock(&kvm->srcu);
>  	hva = gfn_to_hva(kvm, gpa_to_gfn((gpa_t)fib->fmt0.aibv));
>  	npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
> @@ -261,6 +269,12 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  
>  	/* Pin the guest AISB if one was specified */
>  	if (fib->fmt0.sum == 1) {
> +		/* AISB must be dword aligned */
> +		if (fib->fmt0.aisb & 0x7) {
> +			rc = -EINVAL;
> +			goto unpin1;
> +		}
> +

[Severity: High]
This is a pre-existing issue, but this alignment check also jumps past the 
required unregistration. Should unpin1 or a similar cleanup label handle
the GISC unregistration?


[Severity: High]
This is a pre-existing issue, but I noticed there doesn't seem to be a check 
to see if AEN is already enabled on the device.

If a guest repeatedly registers AEN without deregistering first, could the
function allocate new airq_iv structures, pin new guest memory pages, and
overwrite the tracking pointers (zdev->aibv, zdev->aisb), leading to an
exhaustion of host resources?

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


[Severity: Critical]
This is a pre-existing issue, but if a guest registers AEN without requesting a
summary bit (fib->fmt0.sum == 0), does aisb_page remain NULL?

If so, later in kvm_s390_pci_aif_enable() we execute:

	gaite->aisb = virt_to_phys(page_address(aisb_page) + (fib->fmt0.aisb &
							      ~PAGE_MASK));

This appears to perform pointer arithmetic on page_address(NULL), yielding a
garbage physical address. When the device issues an interrupt, could KVM's
aen_host_forward() see that gaite->aisb != 0 and perform a bit flip on that
garbage address, directly corrupting host memory?


[Severity: High]
This isn't a bug introduced by this patch, but does the error handling for
account_mem() and airq_iv_alloc_bit() return a false success and leak
memory accounting?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	/* Account for pinned pages, roll back on failure */
	if (account_mem(pcount))
		goto unpin2;
    ...
	bit = airq_iv_alloc_bit(aift->sbv);
	if (bit == -1UL)
		goto unlock;
    ...
}

In both cases, rc seems to remain unassigned (0). Also, it appears the
unlock error path does not call unaccount_mem(pcount).


[Severity: High]
This is a pre-existing issue, but what happens if airq_iv_create() fails and
returns NULL?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	zdev->aibv = airq_iv_create(msi_vecs, ...);
    ...
}

It seems zdev->aibv is assigned without a check, and subsequently
kvm_zpci_set_airq() will unconditionally dereference it:

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

Could this lead to a NULL pointer dereference panic under host memory pressure?


[Severity: Critical]
This is a pre-existing issue, but is the user-provided aisbo validated
against a 64-bit bounds check?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
	gaite->aisbo = fib->fmt0.aisbo;
    ...
}

If aisbo is unvalidated and can be up to 255, could KVM's software handler
blindly execute a bit flip crossing into unpinned host physical memory?

arch/s390/kvm/interrupt.c:aen_host_forward() {
    ...
		set_bit_inv(gaite->aisbo, phys_to_virt(gaite->aisb));
    ...
}


[Severity: High]
This is a pre-existing issue, but if kvm_zpci_set_airq() fails at the very
end of the function, does it bypass the software rollback?

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:
    ...
}

It looks like it returns immediately instead of executing the error cleanup
labels (unpin1, unpin2, unlock), leaving resources leaked.

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