Re: [PATCH v1 3/5] 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] Hardware DMA out-of-bounds write due to missing page boundary validation for the AIBV.
- [High] Unbounded resource leak and potential Use-After-Free on repeated KVM_S390_ZPCIOP_REG_AEN calls.
- [High] Context mismatch in memory accounting leads to leakage or underflow during asynchronous teardown.
--

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

KVM: s390: pci: Fix NULL dereference on AIBV allocation failure

The commit adds a NULL check for the return value of airq_iv_create() to
prevent a potential NULL pointer dereference. If the allocation fails, it
now properly frees the previously allocated AISB bit before returning an
error.

> --- a/arch/s390/kvm/pci.c
> +++ b/arch/s390/kvm/pci.c
> @@ -299,6 +299,11 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,

[Severity: Critical]
This isn't a regression introduced by this patch, but is there a missing page
boundary validation for the AIBV that could lead to an out-of-bounds DMA
write?

If the guest provides an unaligned guest physical address for the AIBV that
crosses a page boundary, does the pinning logic fail to account for vector
lengths spanning across boundaries?

arch/s390/kvm/pci.c:kvm_s390_pci_aif_enable() {
    ...
    npages = pin_user_pages_fast(hva, 1, FOLL_WRITE | FOLL_LONGTERM, pages);
    ...
}

Could the hardware firmware sequentially DMA interrupt data across the page
boundary into an unpinned host physical page?

>  				    AIRQ_IV_GUESTVEC,
>  				    phys_to_virt(fib->fmt0.aibv));
>  
> +	if (!zdev->aibv) {
> +		rc = -ENOMEM;
> +		goto free_aisb;
> +	}
> +

[Severity: High]
This isn't a regression introduced by this patch, but does this path leak
the airq_iv bit vectors and cause a potential use-after-free?

If a user-space process repeatedly triggers the KVM_S390_ZPCIOP_REG_AEN ioctl,
does this code blindly overwrite zdev->aisb and zdev->aibv without clearing
the global aift->kzdev[old_aisb] reference?

If the device is later deregistered, only the newest AISB index would be
cleared. If a hardware interrupt triggers for the orphaned AISB bit, could
aen_host_forward() access the freed kzdev structure?

>  	spin_lock_irq(&aift->gait_lock);
>  	gaite = aift->gait + zdev->aisb;
>  
> @@ -335,6 +340,9 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>  	rc = kvm_zpci_set_airq(zdev);
>  	return rc;
>  
> +free_aisb:
> +	airq_iv_free_bit(aift->sbv, zdev->aisb);
> +	zdev->aisb = 0;
>  unlock:
>  	if (pcount > 0)
>  		unaccount_mem(pcount);

[Severity: High]
This isn't a regression introduced by this patch, but does unaccount_mem()
incorrectly rely on current->mm in asynchronous teardown contexts?

If unaccount_mem() is called asynchronously (for example, during a VFIO
device removal or sysfs unbind):

arch/s390/kvm/pci.c:unaccount_mem() {
    struct user_struct *user = get_uid(current_user());
    ...
    if (current->mm)
        atomic64_sub(nr_pages, &current->mm->pinned_vm);
    ...
}

Would current point to the worker thread rather than the QEMU process that
originally pinned the memory, leading to an underflow of the wrong memory
quotas and leaking the original quotas?

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