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

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

On 8/31/26 3:40 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [High] Unbounded memory leak of newly pinned NIB pages and GISC registrations when `vfio_ap_irq_enable` times out.
>
> Pre-existing issues:
> - [High] Guest-triggerable WARN_ONCE introduces a host-side denial of service risk.
> --
>
> commit 82e69024e1f94318c978aa31bed7a9d1888b87e2
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
>
> s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable/disable()
>
> This commit modifies the cleanup of pinned NIBs and registered NISCs after
> PQAP(AQIC) instructions are executed. It introduces a wait loop to verify the
> asynchronous completion of the interrupt state transition before allowing
> the release of older resources.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..8fb0476e3d397 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>> +static bool vfio_ap_wait_for_irqstate(int apqn, int ir)
>>   {
>>   	struct ap_queue_status status;
>>   	int retry = 5;
> [ ... ]
>> -	WARN_ONCE(1, "%s: tapq rc %02x: %04x could not clear IR bit\n",
>> -		  __func__, status.response_code, apqn);
>> +	WARN_ONCE(1, "%s: tapq rc %02x: timed out waiting for interrupts %s for %02x.%04x\n",
>> +		  __func__, status.response_code,
>> +		  ir ? "enabled" : "disabled",
>> +		  AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
>> +	return false;
>>   }
> [Severity: High]
> This is a pre-existing issue, but does this WARN_ONCE introduce a guest-driven
> host denial of service risk?
>
> If a guest issues a PQAP(AQIC) instruction and the hardware takes longer than
> expected to transition states, this wait loop times out and triggers a warning.
> Because the KVM subsystem strictly forbids guest-reachable WARN_ON macros,
> could a malicious guest artificially induce these delays (for instance, by
> spamming requests) to intentionally panic the host kernel if panic_on_warn
> is enabled?
>
> [ ... ]
>> @@ -317,8 +334,30 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>>   		switch (status.response_code) {
> [ ... ]
>>   		case AP_RESPONSE_INVALID_ADDRESS:
>>   		default:
>> -			/* All cases in default means AP not operational */
>> +			/*
>> +			 * The AQIC disable was rejected; IRQ is still enabled
>> +			 * and the hardware still holds the NIB address. Do not
>> +			 * free resources.
>> +			 */
>>   			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>>   				  status.response_code);
>> -			goto end_free;
>> +			goto end_fail;
>>   		}
>>   	} while (retries--);
> [Severity: High]
> This is a pre-existing issue, but does this WARN_ONCE pose the same guest-
> driven denial of service risk?
>
> If the hardware returns an unexpected status code in response to a guest-
> initiated disable request, this warning is hit. Can an untrusted guest
> intentionally induce unexpected hardware states to trigger this warning and
> crash the host?
>
> [ ... ]
>> @@ -489,13 +557,49 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
>>   	status = ap_aqic(q->apqn, aqic_gisa, h_nib);
>>   	switch (status.response_code) {
>>   	case AP_RESPONSE_NORMAL:
>> -		/* See if we did clear older IRQ configuration */
>> +		/*
>> +		 * AQIC initiates an asynchronous process; however, AP_RESPONSE_NORMAL
>> +		 * does not guarantee interrupts are enabled yet (i.e., IR bit (7)
>> +		 * is set). Wait to confirm before committing the new NIB and
>> +		 * freeing the old resources.
>> +		 */
>> +		if (!vfio_ap_wait_for_irqstate(q->apqn, AP_IRQ_ENABLED)) {
>> +			/*
>> +			 * Timed out waiting to verify IRQs are enabled. If the
>> +			 * hardware is merely stalled, it might eventually complete
>> +			 * and write interrupt status bytes to the new NIB.
>> +			 *
>> +			 * If the NIB page is unpinned and freed here, this delayed
>> +			 * hardware write would result in a host use-after-free/wild
>> +			 * DMA write and a host kernel crash.
>> +			 *
>> +			 * To prevent this, we must leak the new resources (leave the
>> +			 * NIB page pinned and Guest ISC registered) and return
>> +			 * AP_RESPONSE_OTHERWISE_CHANGED to signal the guest to retry.
>> +			 */
>> +			msg = "%s: Timed out waiting to verify IRQs enabled for apqn=%#04x\n";
>> +			VFIO_AP_DBF_WARN(msg, __func__, q->apqn);
>> +			memset(&status, 0, sizeof(status));
>> +			status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
>> +			break;
>> +		}
> [Severity: High]
> Will this timeout path result in an unbounded memory leak?
>
> When vfio_ap_wait_for_irqstate() times out, the code breaks out of the switch
> statement without storing the newly pinned 'nib' and registered 'isc' into
> q->saved_iova or q->saved_isc. These local variables appear to be permanently
> orphaned.
>
> Because a guest controls the PQAP calls, could an untrusted guest VM repeatedly
> trigger this timeout to deliberately leak pinned host pages and KVM GISC
> registrations until host memory is exhausted?
>
> Additionally, this seems to contradict the commit message which states:
> "The fix is to unregister the NISC and an unpin the NIB used in the AQIC
> call in the default case of the switch statement..."
> Here the code explicitly notes "we must leak the new resources". Is this
> omission of cleanup intentional?

After thorough review of the architecture, it has been determined that
there is no need to wait for the IR bit to clear. The only responsibilities
of the vfio_ap device driver when receiving a mandated exception of
the PQAP-AQIC instruction is to set up the GR2 and GR3 so the queue
interrupts will go to the guest, re-issue the PQAP-AQIC on behalf
of the user and return the CC and the status.response code returned
from the enable command. It is up to the caller on the guest to
check the IR bit to determine when the queue is enabled for
interrupts.

The other responsibility is to manage the pinned page containing the
NIB and the registration of the GISC.

* If AP_RESPONSE_NORMAL is returned from the AQIC, then
    q->saved_iova (the page pinned from the previous enable if it set)
    needs to be unpinned - making it available to another process - and
    the q->saved_isc (the guest ISC registered with the previous enable 
if set)
    needs to be unregistered. The new paged pinned and ISC registered
    before executing the AQIC need to be stored in q.

* For all other response codes, the new NIB needs to be unpinned and
    the new ISC needs to be unregistered since the other response codes
    indicate the AQIC was rejected.


>