Re: [PATCH v3 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/27/26 9:39 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] State Inconsistency / Guest Memory Corruption
>
> Pre-existing issues:
> - [High] Guest-Triggerable Host Denial of Service via WARN_ONCE
> - [Medium] Potential Use-After-Free when replacing an active NIB in `vfio_ap_irq_enable`
> --
>
> commit 01ac8813c3b445f045948ad22e87c958b38cb3b4
> 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 addresses resource leaks by ensuring the NIB is unpinned and the
> NISC is unregistered when the PQAP(AQIC) instruction fails or times out. It
> also introduces a wait loop to prevent a host use-after-free when disabling
> IRQ resources.
>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 940c0ff668bee..64d6a8f8fa96e 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> [ ... ]
>
>> @@ -257,12 +265,13 @@ static void vfio_ap_wait_for_irqclear(int apqn)
>>   		default:
>>   			WARN_ONCE(1, "%s: tapq rc %02x: %04x\n", __func__,
>>   				  status.response_code, apqn);
> [Severity: High]
> This is a pre-existing issue, but could a malicious guest intentionally
> trigger this warning by spamming AP queue instructions? KVM guidelines
> specify that guest-reachable WARN_ONCE statements can act as a local
> denial-of-service vector when panic_on_warn is enabled. Should this be
> converted to pr_warn_once() or dropped entirely?

This branch is reached only when ap_tapq returns a response code
indicating the queue is non-operational (Q_NOT_AVAIL, DECONFIGURED,
CHECKSTOPPED, or an unrecognised code). These conditions are
hardware/firmware-driven; a guest has no architectural mechanism to force
them on demand through normal PQAP instruction execution. The guest cannot
spam its way into a non-operational hardware state.

WARN_ONCE also fires at most once per unique call site for the lifetime of
the kernel, so even if somehow reachable it cannot be used as a repeated
DoS.

>
>> -			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 verifying interrupts disabled for %02x.%04x\n",
>> +		  __func__, status.response_code, AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
> [Severity: High]
> This is a pre-existing issue, but does this WARN_ONCE pose the same
> denial-of-service risk if an untrusted guest intentionally stalls the
> hardware response to exhaust the retry loop?

The retry loop calls ap_tapq, which is a host-privileged instruction
  executed by the host kernel — not the guest. The guest cannot intercept
  or stall ap_tapq execution; that is a host/hypervisor operation. The
  timeout is bounded at 5 × 20 ms = 100 ms regardless of guest behaviour.
  The WARN_ONCE-once-per-site property also applies here.

>
>> +	return false;
>>   }
> [ ... ]
>
>> @@ -317,8 +326,21 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>>   		switch (status.response_code) {
>>   		case AP_RESPONSE_OTHERWISE_CHANGED:
>>   		case AP_RESPONSE_NORMAL:
>> -			vfio_ap_wait_for_irqclear(q->apqn);
>> -			goto end_free;
>> +			/*
>> +			 * AQIC disable was accepted (NORMAL), or the queue was
>> +			 * already disabled or a prior async request is still
>> +			 * completing (OTHERWISE_CHANGED).  In both cases, we must
>> +			 * wait until interrupt processing has been disabled
>> +			 * before proceeding.
>> +			 *
>> +			 * If it could not be determined whether interrupts
>> +			 * have been disabled, do not free the AQIC resources: the
>> +			 * hardware may still write to the NIB, so leave it pinned
>> +			 * to avoid a use-after-free. The resources will be leaked.
>> +			 */
>> +			if (vfio_ap_wait_for_irqclear(q->apqn))
>> +				goto end_free;
>> +			goto end_fail;
> [Severity: High]
> If vfio_ap_wait_for_irqclear() times out, the code correctly takes the
> end_fail path to prevent a host use-after-free. However, doesn't this
> result in returning the original AP_RESPONSE_NORMAL status back to the
> guest?
>
> If the guest receives a success status, it might assume interrupts are
> fully disabled and proceed to free and reuse the memory containing the NIB.
> Since the hardware may still be actively writing to the NIB, could this
> corrupt the guest's re-allocated memory? Should the driver override the
> return status (e.g., returning AP_RESPONSE_BUSY) to force the guest to
> retry instead?
>
>>   		case AP_RESPONSE_RESET_IN_PROGRESS:
>>   		case AP_RESPONSE_BUSY:
>>   			msleep(20);
>> @@ -326,18 +348,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);
> [Severity: High]
> This is a pre-existing issue, but does this WARN_ONCE similarly allow an
> untrusted guest to trigger a kernel panic if panic_on_warn is enabled?

These response codes reflect hardware/firmware-reported non-operational
  states that the guest cannot manufacture on demand. WARN_ONCE fires at
  most once. Not a practical DoS vector in production.

>
>> +			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);
> [Severity: High]
> This is a pre-existing issue, but could this WARN_ONCE also act as a host
> denial-of-service vector if a guest provides invalid queue states?

AP_RESPONSE_INVALID_ADDRESS from ap_aqic means the host-supplied NIB
  address is invalid — this is a value the host driver passes, specifically
  the h_nib derived from vfio_pin_pages. The guest cannot directly control
  the host physical address used here. WARN_ONCE again fires at most once.
  Not a guest-reachable DoS path.

>
>> -			goto end_free;
>> +			goto end_fail;
>>   		}
>>   	} while (retries--);
>>   
>>   	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>>   		  status.response_code);
> [Severity: High]
> This is a pre-existing issue, but could an adversarial guest intentionally
> induce a timeout to hit this WARN_ONCE as well?

This fires only when ap_aqic itself keeps returning BUSY or
  RESET_IN_PROGRESS for all 5 retries × 20 ms. The AQIC instruction is
  issued by the host kernel against a hardware queue. A guest cannot
  artificially hold the AQIC response in BUSY state across all
  retries — that is a hardware/firmware concern. WARN_ONCE fires once.

>
> [ ... ]
>
>> @@ -490,6 +540,11 @@ 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 */
>>   		vfio_ap_free_aqic_resources(q);
>>   		q->saved_iova = nib;
>>   		q->saved_isc = isc;
>>   		break;
> [Severity: Medium]
> This is a pre-existing issue, but if ap_aqic() accepts a new NIB replacement
> and returns AP_RESPONSE_NORMAL, does immediately freeing the old resources
> via vfio_ap_free_aqic_resources() pose a use-after-free risk?

No interrupts are made pending once AP_RESPONSE_NORMAL (cc=0)
is returned; however, in the unlikely event the queue was already enabled
for interrupts, there is no risk of use-after-free for the old NIB. On the
other hand, the new NIB does not get stored by the hardware, until
the I-bit (bit 7) is set in the APQSW, so it would make sense to verify that
before freeing the old NIB.

>
> If the hardware processes the change asynchronously, might it still have
> in-flight DMA writes directed to the old NIB (stored in q->saved_iova)? If
> so, does the old NIB need to be fully quiesced using a similar wait loop
> before it can be safely unpinned?

As stated above, no interrupts are made pending once
AP_RESPONSE_NORMAL (cc=0) is returned. What needs to be done, however,
is to wait until the I-bit (bit 7) is set before clearing the old NIB if 
in fact
it has been store for a previous enable. When the I-bit is set, the 
queue will
set the new NIB, so new writes will be made to the new NIB and the old
will be stale and can be freed.

The fix is to wait for the I-bit to be set before freeing an old NIB.

>
>>   	case AP_RESPONSE_OTHERWISE_CHANGED:
>> -		/* We could not modify IRQ settings: clear new configuration */