Re: [PATCH v8 1/6] s390/vfio-ap: Fix leaks of pinned NIB and registered GISC

[Anthony Krowiak <akrowiak@linux.ibm.com>]

On 9/25/26 8:57 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] s390/vfio-ap: wild DMA write due to unconditional NIB unpin in unmap_iova
> --
>
> commit f138c70af93258555b6e1c1c1233b5d5dc8b82da
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix leaks of pinned NIB and registered GISC
>
> This commit addresses resource leaks of pinned NIB pages and registered
> GISCs across several paths. It introduces intentional leaks for cases where
> hardware might still be able to write to the NIB, while freeing resources
> when queues become unavailable.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> @@ -1857,8 +2006,28 @@ static void unmap_iova(struct ap_matrix_mdev *matrix_mdev, u64 iova, u64 length)
>>   	int loop_cursor;
>>   
>>   	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
>> -		if (q->saved_iova >= iova && q->saved_iova < iova + length)
>> +		if (q->saved_iova >= iova && q->saved_iova < iova + length) {
>>   			vfio_ap_irq_disable(q);
>> +			/*
>> +			 * If IRQ disable failed or IR=0 could not be confirmed,
>> +			 * vfio_ap_irq_disable() intentionally leaks the NIB to
>> +			 * prevent a wild DMA write. But vfio core requires the
>> +			 * page to be unpinned before dma_unmap returns, or it
>> +			 * will BUG_ON after 10 re-notification rounds.
>> +			 *
>> +			 * Fall back to a bounded queue reset. The ZAPQ zeroizes
>> +			 * the NIB pointer in hardware, eliminating the DMA risk
>> +			 * that justified the leak. Once the worker finishes (or
>> +			 * times out with a reset confirmed in-progress), the
>> +			 * hardware no longer holds a reference to saved_iova and
>> +			 * it is safe to unpin unconditionally.
>> +			 */
>> +			if (q->saved_iova) {
>> +				vfio_ap_mdev_reset_queue(q);
>> +				flush_work(&q->reset_work);
>> +				vfio_ap_free_aqic_resources(q);
> [Severity: High]
> Does this unconditional call to vfio_ap_free_aqic_resources() risk allowing
> a wild DMA write into host memory?
>
> If vfio_ap_mdev_reset_queue() encounters a hardware fault and TAPQ returns
> an invalid response code, it intentionally leaves the NIB pinned. The
> comment in vfio_ap_mdev_reset_queue() notes:
>
> "An invalid response code indicates a hardware or firmware bug. Since we
> cannot determine whether the queue can still DMA-write to the NIB, the AQIC
> resources are intentionally leaked lest the NIB page is reallocated to a new
> owner."
>
> However, calling vfio_ap_free_aqic_resources() in unmap_iova() forces the
> NIB page to be unpinned regardless of the reset result. If the ZAPQ failed
> to zeroize the NIB, unpinning it here overrides the intentional safety leak
> and could expose the host to memory corruption.

vfio_ap_reset_queue() along with the apq_reset_check() work queue function
frees the AQIC resources only if reset completion is verified; so, the
freeing of resources should be delegated to vfio_ap_reset_queue()
and not unconditionally.

>
>> +			}
>> +		}
>>   	}
>>   }