Re: [PATCH v2 3/4] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Guest-controlled AIBV can cross a page boundary, leading to host memory corruption.
- [High] Missing cleanup when kvm_zpci_set_airq() fails in kvm_s390_pci_aif_enable().
- [High] Missing state check allows unbounded resource leaks via repeated KVM_S390_ZPCIOP_REG_AEN ioctls.
--

--- Patch [3]: [PATCH v2 3/4] KVM: s390: pci: Fix NULL dereference on AIBV allocation failure ---
commit 6cf8d654914206b2e0a5d8af6cb9a7272f96c280
Author: Farhan Ali <alifm@linux.ibm.com>
KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

This commit adds a check to ensure airq_iv_create() does not return NULL,
preventing a subsequent NULL pointer dereference in kvm_zpci_set_airq().
If the allocation fails, it frees the previously allocated AISB bit and
returns -ENOMEM.

> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -240,6 +240,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  				   bool assist)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing state
check here before proceeding with the allocations?

If the ioctl for AEN registration is called repeatedly for the same PCI
device, this function appears to unconditionally pin new memory pages,
allocate new aift->sbv bits, and overwrite the previous pointers in zdev
without freeing them. Could this lead to exhausting aift->sbv bits and
leaking pinned memory pages?

>  	struct page *pages[1], *aibv_page, *aisb_page = NULL;
>  	unsigned int msi_vecs, idx;

[ ... ]

>  	/* Replace AIBV address */
>  	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);
>  	srcu_read_unlock(&kvm->srcu, idx);
>  	if (npages < 1) {
>  		rc = -EIO;
>  		goto out;
>  	}
>  	aibv_page = pages[0];
>  	pcount++;
>  	gaddr = page_to_phys(aibv_page) + (fib->fmt0.aibv & ~PAGE_MASK);
>  	fib->fmt0.aibv = gaddr;

[Severity: Critical]
This is a pre-existing issue, but does this calculation safely handle the
case where the bit vector crosses a page boundary?

The code pins exactly one page, but the interrupt bit vector size depends
on msi_vecs. If fib->fmt0.aibv is provided near the end of a page
boundary (for example, offset 4088) and msi_vecs requires more bytes than
what is left in the page, will the hardware and host software read or write
out-of-bounds into the adjacent unmapped physical page?

[ ... ]

> @@ -346,6 +351,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	/* Save some guest fib values in the host for later use */
>  	zdev->kzdev->fib.fmt0.isc = fib->fmt0.isc;
>  	zdev->kzdev->fib.fmt0.aibv = fib->fmt0.aibv;
>  
>  	/* Issue the clp to setup the irq now */
>  	rc = kvm_zpci_set_airq(zdev);
>  	return rc;

[Severity: High]
This isn't a bug introduced by this patch, but does returning directly
here bypass the required cleanup logic?

If kvm_zpci_set_airq() fails, returning rc directly means we skip the
free_aisb and unlock labels. Could this leave the pages pinned, the
accounted memory incremented, and the aift->sbv bits allocated, causing
these resources to be permanently lost?

>  
> +free_aisb:
> +	airq_iv_free_bit(aift->sbv, zdev->aisb);
> +	zdev->aisb = 0;
>  unlock:
>  	if (pcount > 0)

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