Re: [PATCH v6 1/5] s390/vfio-ap: Fix leak of pinned NIB and registered GISC in vfio_ap_irq_enable/disable()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Guest-reachable WARN_ONCE macros violate KVM guidelines and can cause a host denial of service (DoS) under panic_on_warn.

Pre-existing issues:
- [High] Pinned NIB page and registered GISC are permanently leaked when a queue is removed from the host AP configuration or becomes physically unavailable.
--

commit ea9124e6c06c371c42de8146e0e3493413dc0829
Author: Anthony Krowiak <akrowiak@linux.ibm.com>
s390/vfio-ap: Fix leak of pinned NIB and registered GISC in vfio_ap_irq_enable/disable()

This commit updates the interrupt enable and disable handlers for vfio_ap
to correctly manage pinned NIB pages and GISC registrations. It introduces 
an asynchronous wait process for interrupt clearing to prevent wild DMA 
writes, leaving resources allocated if the hardware doesn't confirm the 
operation.

> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 940c0ff668bee..383ec9f5c8107 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c

[ ... ]

> @@ -257,12 +269,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 -ENODEV;
>  		}
>  	} 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 disabled for %02x.%04x\n",
> +		  __func__, status.response_code,
> +		  AP_QID_CARD(apqn), AP_QID_QUEUE(apqn));
> +
> +	return -ETIMEDOUT;
>  }

[Severity: High]
Does this allow an unprivileged guest to trigger a host panic?

According to KVM subsystem guidelines, guest-reachable WARN_ON or WARN_ONCE
macros are prohibited because they can crash the host kernel on systems
configured with panic_on_warn.

If a guest forces a delay by issuing a ZAPQ reset (which can take up to
2000ms), and immediately follows it with a PQAP-AQIC disable instruction,
this 100ms TAPQ polling loop (5 retries * 20ms) will time out. 

Could this be converted to pr_warn_once() to avoid host denial of service?

[ ... ]

> @@ -326,18 +372,47 @@ 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;
>  		}
>  	} while (retries--);

[Severity: High]
Does this share the same risk of triggering a host panic as the warning above?

Similar to the timeout case, if the AP returns statuses like DECONFIGURED
or CHECKSTOPPED, this guest-reachable path triggers a WARN_ONCE. Should
these be converted to pr_warn_once() to prevent a potential host DoS?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does the vfio_ap driver leak
the pinned NIB page and registered GISC when a queue is physically removed
or becomes unavailable?

Looking at vfio_ap_mdev_remove_queue(), if a queue with active interrupts is
removed from the host AP configuration, the bit test fails and we skip
calling vfio_ap_mdev_reset_queue(q):

	/*
	 * If the queue is not in the host's AP configuration, then resetting
	 * it will fail with response code 01, (APQN not valid); so, let's make
	 * sure it is in the host's config.
	 */
	if (test_bit_inv(apid, (unsigned long *)matrix_dev->info.apm) &&
	    test_bit_inv(apqi, (unsigned long *)matrix_dev->info.aqm)) {
		vfio_ap_mdev_reset_queue(q);
		flush_work(&q->reset_work);
	}

done:
	if (matrix_mdev)
		vfio_ap_unlink_queue_fr_mdev(q);

	dev_set_drvdata(&apdev->device, NULL);
	kfree(q);

Because the reset is bypassed, vfio_ap_free_aqic_resources(q) is never
called, permanently leaking the page and KVM routing entries.

Similarly, in vfio_ap_mdev_reset_queue():

	default:
		WARN(true,
		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
		     status.response_code);
	}

If ap_zapq() returns AP_RESPONSE_Q_NOT_AVAIL (0x01) because the queue is 
physically unavailable, the default case is hit. It logs a warning but fails 
to call vfio_ap_free_aqic_resources(q), leaking the resources.

Could this lead to memory exhaustion and depletion of KVM ISCs over time if
devices are dynamically reconfigured?

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