[PATCH v3 3/4] s390/vfio-ap: Fix unbounded loop in apq_reset_check()

[Anthony Krowiak]

The apq_reset_check() worker polls ap_tapq() in a while(true) loop
waiting for a queue reset to complete. When ap_tapq() returns
AP_RESPONSE_BUSY or AP_RESPONSE_RESET_IN_PROGRESS,
apq_status_check() returns -EBUSY and the loop continues after
sleeping AP_RESET_MAX_WAIT (20ms). There is no upper bound on how
many times the loop iterates, so if the hardware continuously
returns a busy response the worker runs indefinitely.

This is particularly harmful because several callers of
vfio_ap_mdev_reset_queues() and vfio_ap_mdev_reset_qlist() call
flush_work() on each queue's reset_work while holding one or more
of the global matrix_dev locks (guests_lock, mdevs_lock) or the
KVM lock. An indefinitely spinning worker permanently blocks all
of those locks, hanging mdev removal, KVM guest teardown, and the
VFIO_DEVICE_RESET ioctl path.

Fix this by introducing AP_RESET_MAX_WAIT (2000ms) and breaking out
of the poll loop when elapsed time reaches that threshold. On
timeout the final busy status is written back to q->reset_status
so that callers inspecting reset_status.response_code after
flush_work() see a non-zero value and can return an appropriate
error.

The AQIC resources associated with this queue - the pinned page
containing the NIB and the registered guest ISC - cannot be freed
in this case. The NIB is the active DMA target for AP interrupt
delivery until the reset completes; freeing the pinned page while the
hardware may still write to it would result in a use-after-free
kernel crash. If the reset eventually completes, interrupts will be
terminated, but the pinned NIB page and ISC registration will be leaked.
This is preferable to either a use-after-free or waiting indefinitely.

Fixes: dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue reset to complete")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Anthony Krowiak <akrowiak@xxxxxxxxxxxxx>
---
 drivers/s390/crypto/vfio_ap_ops.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 4c32fd6eaaa6..a9a33f4949a0 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -31,6 +31,7 @@
 #define AP_QUEUE_IN_USE "in use"
 
 #define AP_RESET_INTERVAL		20	/* Reset sleep interval (20ms)		*/
+#define AP_RESET_MAX_WAIT		2000	/* Maximum wait for reset (2000ms)	*/
 
 static int vfio_ap_mdev_reset_queues(struct ap_matrix_mdev *matrix_mdev);
 static int vfio_ap_mdev_reset_qlist(struct list_head *qlist);
@@ -2016,8 +2017,32 @@ static void apq_reset_check(struct work_struct *reset_work)
 		elapsed += AP_RESET_INTERVAL;
 		status = ap_tapq(q->apqn, NULL);
 		ret = apq_status_check(q->apqn, &status);
-		if (ret == -EIO)
+		if (ret == -EIO) {
+			memcpy(&q->reset_status, &status, sizeof(status));
 			return;
+		}
+		if (elapsed >= AP_RESET_MAX_WAIT) {
+		/*
+		 * Timed out waiting for reset to complete.
+		 *
+		 * The AQIC resources associated with this queue - the pinned page
+		 * containing the NIB and the registered guest ISC - cannot be freed
+		 * here. The NIB is the active DMA target for AP interrupt delivery
+		 * until the reset completes; freeing the pinned page while the
+		 * hardware may still write to it would result in a use-after-free
+		 * kernel crash.
+		 *
+		 * If the reset eventually completes, interrupts will be terminated
+		 * and the pinned NIB page and ISC registration will be leaked. This
+		 * is preferable to either a use-after-free or waiting indefinitely:
+		 * the caller of apq_reset_check() holds mdevs_lock while flush_work()
+		 * blocks holds the matrix_dev->mdevs_lock mutex, which
+		 * serializes access to all mdev objects system-wide, so blocking
+		 * here would stall all other guests using AP queues.
+		 */
+			memcpy(&q->reset_status, &status, sizeof(status));
+			return;
+		}
 		if (ret == -EBUSY) {
 			pr_notice_ratelimited(WAIT_MSG, elapsed,
 					      AP_QID_CARD(q->apqn),
-- 
2.53.0