Re: [PATCH v4 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/28/26 5:57 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Critical] Prematurely unpinning the new NIB page on a hardware timeout in `vfio_ap_irq_enable` causes a host use-after-free and wild DMA write if the hardware later completes the asynchronous interrupt configuration.
>
> Pre-existing issues:
> - [High] Guest-reachable `WARN_ONCE` in `vfio_ap_wait_for_irqstate` and `vfio_ap_irq_disable` violates KVM subsystem guidelines and creates a denial of service vector on `panic_on_warn` systems.
> --
>
> commit f514ef27dfc693d5c326ffed4fd1a9985acce1d0
> Author: Anthony Krowiak <akrowiak@linux.ibm.com>
> Subject: s390/vfio-ap: Fix leak of pinned NIB and registered NISC in vfio_ap_irq_enable/disable()
>
> This commit addresses resource leaks by restructuring how the AQIC
> instruction responses are handled for AP queues. It introduces a polling
> mechanism to wait and confirm asynchronous hardware transitions before freeing
> resources.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..24e93fb7f81a2 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -245,8 +259,8 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>>   		switch (status.response_code) {
>>   		case AP_RESPONSE_NORMAL:
>>   		case AP_RESPONSE_RESET_IN_PROGRESS:
>> -			if (!status.irq_enabled)
>> -				return;
>> +			if (status.irq_enabled == ir)
>> +				return true;
>>   			fallthrough;
>>   		case AP_RESPONSE_BUSY:
>>   			msleep(20);
>> @@ -257,12 +271,15 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>>   		default:
>>   			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
>>   				  status.response_code, apqn);
>> -			return;
>> +			return true;
>>   		}
>>   	} while (--retry);
>>   
>> -	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 isn't a bug introduced by this patch, but does this guest-reachable
> warning violate KVM subsystem guidelines?
>
> A malicious guest can intentionally manipulate AP queue states or rapidly
> spam PQAP instructions to stall the hardware, triggering these warnings on
> non-operational states or polling timeouts. On systems configured with
> panic_on_warn, this creates a host denial of service vector.
>
> Could these be converted to rate-limited debug messages or dropped entirely?

Previously responded to; not a problem

>
> [ ... ]
>
>> @@ -326,18 +365,46 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>>   		case AP_RESPONSE_Q_NOT_AVAIL:
>>   		case AP_RESPONSE_DECONFIGURED:
>>   		case AP_RESPONSE_CHECKSTOPPED:
>> +			/* AP not operational; no further interrupts possible */
>> +			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>> +				  status.response_code);
>> +			goto end_free;
>>   		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;
>>   		}
> [Severity: High]
> This is also a pre-existing issue, but can this similarly be triggered by
> an unprivileged guest manipulating the queue into an unavailable state during
> a disable attempt?

No. Unprivileged guests cannot transition or spoof a queue into
these states (AP configuration is managed strictly at the host/
LPAR level).

Furthermore, when a queue physically enters a non-operational
state (such as DECONFIGURED or CHECKSTOPPED), the hardware fully
halts the queue and aborts all pending operations. Because no
further DMA writes to the NIB can occur, freeing the resources
under these response codes is entirely safe.

>
> [ ... ]
>
>> @@ -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 the IR bit is set yet.  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: the hardware may not have accepted the new
>> +			 * NIB.  Clean up the new resources and return
>> +			 * OTHERWISE_CHANGED to signal the guest to retry.
>> +			 */
>> +			ret = kvm_s390_gisc_unregister(kvm, isc);
>> +			if (ret) {
>> +				msg = "%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n";
>> +				VFIO_AP_DBF_WARN(msg, __func__, ret, isc, q->apqn);
>> +			}
>> +			vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
>> +			memset(&status, 0, sizeof(status));
>> +			status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
>> +			break;
>> +		}
> [Severity: Critical]
> Does unpinning the new NIB page here introduce a race that could lead to a
> wild DMA write and host memory corruption?
>
> Because ap_aqic() returned AP_RESPONSE_NORMAL, the hardware accepted the
> asynchronous request. If the hardware is merely stalled (e.g., due to a
> malicious guest spamming instructions) and vfio_ap_wait_for_irqstate() times
> out, the hardware might still eventually complete the transition and write
> interrupt status bytes to the new NIB.
>
> If vfio_unpin_pages() is called here, that physical page could be reallocated
> by the host kernel for a different purpose before the delayed hardware DMA
> write completes.

This is a legitimate concern and needs to be fixed.

>