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?

Responded to with v3, not legitimate.

>
> [ ... ]
>
>> @@ -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?

Responded to with v3, not legitimate.


>
> [ ... ]
>
>> @@ -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.

Legitimate concern, will be fixed in v5

>