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

On 8/24/26 6:07 PM, [email protected] wrote:
> 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?

This is a valid concern and is similar to the problem fixed by this patch.
It was also introduced by the same patch identified in the Fixes: tag,
so I will add the fix for the issue pointed out above to this patch and
resubmit it.

>
>> +		/* 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;
>>   	}