Re: [PATCH v5 6/6] KVM: s390: pci: Validate AIBV and AISB before pinning guest pages

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The AIBV size calculation underestimates the required memory footprint when the bit vector has a non-zero bit offset, allowing an unaligned AIBV to silently cross a page boundary and cause host memory corruption via hardware DMA.

Pre-existing issues:
- [High] Guest AIBV and AISB pages are unpinned without being marked dirty, leading to potential silent data loss of hardware DMA writes.
--

--- Patch [6]: [PATCH v5 6/6] KVM: s390: pci: Validate AIBV and AISB before pinning guest pages ---
commit ce58b81776049069915a6d5914d02bc982861691
Author: Farhan Ali <alifm@linux.ibm.com>

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

The AIBV holds one bit per MSI-X vector for a given function. If the size
of the AIBV exceeds a single page boundary, or if the AISB address is not
8-byte aligned, the request is rejected to prevent unsafe pinning of the
guest pages.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index ca8ef76bcf6b3..25185d04ca28e 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c

[ ... ]

> @@ -271,6 +271,15 @@ 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) * sizeof(unsigned long);
> +	size += (fib->fmt0.aibvo / 8);
> +	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);

[Severity: Critical]
Does this calculation underestimate the true byte footprint of the bit vector
when aibvo is non-zero?

If a guest provides msi_vecs = 64, aibvo = 63, and an aibv GPA with a page
offset of 4081, the calculated size is 15 bytes instead of 16. The npages
calculation yields DIV_ROUND_UP(4081 + 15, 4096) = 1, which incorrectly
passes the npages > 1 boundary check.

This would result in KVM pinning only one page, but the hardware could access
the 16th byte, writing to the first byte of the next unpinned host physical
page, which could lead to host memory corruption.

> +	/* AIBV cannot span more than 1 page */
> +	if (npages > 1) {
> +		rc = -EINVAL;
> +		goto out;
> +	}
> +
>  	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);

[Severity: High]
This isn't a bug introduced by this patch, but does the unpinning of guest
AIBV and AISB pages in kvm_s390_pci_aif_disable() need to mark the pages
as dirty?

The pages are pinned with FOLL_WRITE in kvm_s390_pci_aif_enable(), and PCI
hardware modifies the pinned pages directly via DMA. When unpinning these
pages later in kvm_s390_pci_aif_disable():

arch/s390/kvm/pci.c:kvm_s390_pci_aif_disable() {
    ...
	if (vpage) {
		unpin_user_page(vpage);
		pcount++;
	}
	if (spage) {
		unpin_user_page(spage);
		pcount++;
	}
    ...
}

Without marking them dirty, if the host's memory management subsystem
later reclaims or swaps out these pages, the hardware DMA writes could be
silently lost. Could this lead to lost interrupts and guest malfunctions?

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