Re: [PATCH v2 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable()

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

Pre-existing issues:
- [Critical] Potential host memory corruption if `vfio_ap_irq_disable` times out while the hardware queue is busy.
--

--- Patch [1]: [PATCH v2 1/4] s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable() ---
commit 85cfee5e61842e2a3b3b8993da6d14f25c2209a2
Author: Anthony Krowiak <[email protected]>

s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable()

This commit fixes a resource leak in vfio_ap_irq_enable() by explicitly
unregistering the NISC and unpinning the NIB in the default switch case,
rather than relying on vfio_ap_irq_disable().

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..22a6ceaa7b562 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -503,9 +503,12 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>  		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>  		break;
>  	default:
> -		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> -			status.response_code);
> -		vfio_ap_irq_disable(q);

[Severity: Critical]
This isn't a bug introduced by this patch, but pre-exists in the
vfio_ap_irq_disable() function that was previously called here.

Can this lead to host memory corruption if vfio_ap_irq_disable() times out
while the hardware queue is busy?

If a guest repeatedly sends requests to keep the AP queue busy, then issues a
PQAP(AQIC) instruction with the IR bit cleared (or unmaps memory),
vfio_ap_irq_disable() is invoked. If the queue remains busy, ap_aqic() returns
AP_RESPONSE_BUSY repeatedly.

After retries, vfio_ap_irq_disable() falls through to end_free:

drivers/s390/crypto/vfio_ap_ops.c:vfio_ap_irq_disable() {
	...
	} while (retries--);

	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
		  status.response_code);
end_free:
	vfio_ap_free_aqic_resources(q);
}

By unconditionally unpinning the NIB (Notification Indicator Byte) page via
vfio_ap_free_aqic_resources() while the hardware still holds its address and
expects to write to it, the page may be reallocated to another process.

When the hardware eventually triggers an interrupt, wouldn't it perform a DMA
write to this freed page, causing host memory corruption?

> +		/* We could not modify IRQ settings: clear new configuration */
> +		ret = kvm_s390_gisc_unregister(kvm, isc);
> +		if (ret)
> +			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
> +					 __func__, ret, isc, q->apqn);
> +		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>  		break;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1