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

[sashiko-bot@kernel.org]

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?

> -			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?

> +	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?

> +			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?

> -			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?

[ ... ]

> @@ -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?

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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827132441.555866-1-akrowiak@linux.ibm.com?part=1