Re: [PATCH v1 5/5] KVM: s390: pci: Fix AIBV and AISB spanning multiple pages

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Multi-page pinning relies on a single host virtual address (HVA) translation, which fails across KVM memslot boundaries.

Pre-existing issues:
- [Critical] Hardware DMA and kernel virtual accesses overflow into unrelated physical pages due to lack of physical contiguity.
- [High] Repeated registration ioctls leak pinned memory pages and `airq_iv` structures.
--

--- Patch [5]: [PATCH v1 5/5] KVM: s390: pci: Fix AIBV and AISB spanning multiple pages ---
commit 4f8b3d86d13ac2b1f9a5979bfc8a008742cfc9de
Author: Farhan Ali <alifm@linux.ibm.com>

KVM: s390: pci: Fix AIBV and AISB spanning multiple pages

This patch modifies KVM s390 pci interrupt forwarding to pin multiple pages
for AIBV and AISB structures if they cross a page boundary. It calculates the
required number of pages and pins them to handle unbounded sizes or varying
alignments correctly.

> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
> index 9a16a2eac65b8..5fe07181f3d6a 100644
> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -229,13 +229,15 @@ static inline int account_mem(unsigned long nr_pages)
>  static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  				   bool assist)
>  {

[ ... ]

>  	/* Replace AIBV address */
> +	size = BITS_TO_LONGS(msi_vecs) * sizeof(unsigned long);
> +	npages = DIV_ROUND_UP((fib->fmt0.aibv & ~PAGE_MASK) + size, PAGE_SIZE);
> +	if (npages > MAX_AIF_PAGES) {
> +		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);
> +	npinned = pin_user_pages_fast(hva, npages, FOLL_WRITE | FOLL_LONGTERM,
> +				      kzdev->aibv_pages);

[Severity: Medium]
If the AIBV crosses a page boundary that aligns with a KVM memslot boundary
where the adjacent memslot maps to a disjoint HVA region, won't this pin the
wrong host memory pages since we only translate the first GFN to an HVA?

[Severity: High]
This is a pre-existing issue, but if a guest repeatedly invokes the
registration ioctl for the same zPCI device, won't this leak pinned memory
pages by overwriting kzdev->aibv_pages without releasing the previous pages?

Similarly, when zdev->aibv is overwritten later 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));

Does this leak the previous airq_iv structure and exhaust adapter interrupt
bit vectors?

>  	srcu_read_unlock(&kvm->srcu, idx);
> -	if (npages < 1) {
> +	if (npinned <= 0) {
>  		rc = -EIO;
>  		goto out;
>  	}
> -	aibv_page = pages[0];
> -	pcount++;
> +
> +	kzdev->aibv_npages = npinned;
> +	if (npinned < npages) {
> +		rc = -EIO;
> +		goto unpin1;
> +	}
> +
> +	pcount += npinned;
> +	aibv_page = kzdev->aibv_pages[0];
>  	gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
>  	fib->fmt0.aibv = gaddr;

[Severity: Critical]
This is a pre-existing issue, but if npinned is greater than 1 and the user
pages are not physically contiguous, won't providing a single base physical
address (gaddr) cause hardware DMA accesses to linearly overflow into
unrelated physical memory adjacent to aibv_page?

Also, later 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));

Does phys_to_virt(fib->fmt0.aibv) similarly cause kernel virtual accesses
to corrupt unrelated physical memory if the pages are not contiguous?

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