[PATCH v6 5/5] s390/vfio-ap: fix queue state leakage to guest and host

[Anthony Krowiak <akrowiak@linux.ibm.com>]

Commit dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue
reset to complete") removed the upper bound on the wait for a queue reset
to complete in apq_reset_check(), thus allowing the function to loop
indefinitely. The reason given was to ensure both the security requirements
and prevent resource leakage and corruption in the hypervisor.

That is a legitimate concern; however, functions initiating the reset all
hold the matrix_dev->mdevs_lock which guards access to all of the mdevs
under the control of the vfio_ap device driver. Blocking of access prevents
a system administrator from configuring the mdevs (i.e.,
assigning/unassigning adapters, domains and control domains via the mdev's
sysfs interfaces) and may hang any guest that is started using one of
the mdevs to supply its AP configuration.

This patch addresses two issues:
1. Limits the potential hang condition to a single mdev/guest.

2. Prevents leakage of the internal state of a queue to the host or
   any guest started using an mdev to supply its AP configuration.

The key code changes made to address these issues:

1. _queue_passable() accepted reset status response codes
   AP_RESPONSE_DECONFIGURED and AP_RESPONSE_CHECKSTOPPED as passable
   states in addition to AP_RESPONSE_NORMAL. Neither DECONFIGURED nor
   CHECKSTOPPED confirms that the queue was zeroized; only
   AP_RESPONSE_NORMAL (0) - set by apq_reset_check() after TAPQ status bit
   verification - does. To fix this, the passable condition will be limited
   to AP_RESPONSE_NORMAL only.

2. Added a reset_max_wait field to struct vfio_ap_queue. This value
   specifies the maximum length of time to wait for reset completion. A
   value of zero indicates wait until the reset completes.

3. apq_reset_check() used the compile-time constant AP_RESET_MAX_WAIT
   as a hard timeout, causing the reset polling loop to give up and
   mark the queue non-normal even when the caller required confirmed
   zeroization. Replace the constant with the reset_max_wait field from
   the vfio_ap_queue object. When set to AP_RESET_MAX_WAIT, the behaviour
   is unchanged; when set to 0 (new unbind path) the loop runs until
   zeroization is confirmed regardless of elapsed time.

4. Introducing a reset_max_wait value of zero to mean "wait indefinitely"
   required changes to apq_reset_check() beyond simply replacing the
   AP_RESET_MAX_WAIT constant. When the max wait block is bypassed, the
   function must still exit the loop when apq_status_check() returns 0
   (zeroization confirmed) or -ENODEV (device gone), so an explicit
   goto done is added for those two cases. Additionally, the -EBUSY/-EAGAIN
   re-ZAPQ branch used continue followed by an unconditional goto done,
   which caused the loop to exit immediately after a ZAPQ retry rather
   than continuing to poll; both are removed so polling continues
   correctly when the max wait block is bypassed.

5. vfio_ap_mdev_probe_queue() only zeroed the reset_status field at
   probe time; it did not reset the queue itself, so a queue inherited
   whatever state the firmware left it in. Call vfio_ap_mdev_reset_queue()
   and flush_work() at probe to guarantee a clean queue before it can
   be assigned to a guest. Symmetrically, vfio_ap_mdev_remove_queue()
   freed the queue structure immediately after unlinking it even if the
   queue had not been confirmed zeroized. Delay kfree() until after a
   blocking reset (reset_max_wait = 0) when the queue is not already
   confirmed clean, preventing host exposure to stale queue state.
   This, of course, will block the unbinding of the queue until zeroization
   is confirmed, but that is preferable to blocking access to all of the
   mdevs under the vfio_ap device driver's control.

Fixes: dd174833e44e ("s390/vfio-ap: remove upper limit on wait for queue reset to complete")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 39 ++++++++++++++++++---------
 drivers/s390/crypto/vfio_ap_private.h |  3 +++
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 6a964f82c8e8..e054fd4a9497 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -781,14 +781,14 @@ static bool _queue_passable(struct vfio_ap_queue *q)
 	if (!q)
 		return false;
 
-	switch (q->reset_status.response_code) {
-	case AP_RESPONSE_NORMAL:
-	case AP_RESPONSE_DECONFIGURED:
-	case AP_RESPONSE_CHECKSTOPPED:
-		return true;
-	default:
-		return false;
-	}
+	/*
+	 * A queue is only passable if zeroization was confirmed by
+	 * apq_reset_check() via TAPQ status bit verification. This is
+	 * indicated by reset_status.response_code == AP_RESPONSE_NORMAL (0).
+	 * This is to protect against leaking the internal state of the queue
+	 * to the guest.
+	 */
+	return q->reset_status.response_code == AP_RESPONSE_NORMAL;
 }
 
 /*
@@ -2113,7 +2113,7 @@ static void apq_reset_check(struct work_struct *reset_work)
 		ret = apq_status_check(q->apqn, &status);
 		if (ret == -EIO)
 			return;
-		if (elapsed >= AP_RESET_MAX_WAIT) {
+		if (q->reset_max_wait && elapsed >= q->reset_max_wait) {
 			/*
 			 * Zeroization confirmed (ret == 0): the TAPQ status bits
 			 * indicate the async portion of the ZAPQ completed
@@ -2161,6 +2161,8 @@ static void apq_reset_check(struct work_struct *reset_work)
 
 			return;
 		}
+		if (!ret || ret == -ENODEV)
+			goto done;
 		if (ret == -EBUSY) {
 			pr_notice_ratelimited(WAIT_MSG, elapsed,
 					      AP_QID_CARD(q->apqn),
@@ -2175,9 +2177,7 @@ static void apq_reset_check(struct work_struct *reset_work)
 			    ret == -EAGAIN) {
 				status = ap_zapq(q->apqn, 0);
 				memcpy(&q->reset_status, &status, sizeof(status));
-				continue;
 			}
-			goto done;
 		}
 	}
 
@@ -2675,8 +2675,10 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 
 	q->apqn = apqn;
 	q->saved_isc = VFIO_AP_ISC_INVALID;
-	memset(&q->reset_status, 0, sizeof(q->reset_status));
+	q->reset_max_wait = AP_RESET_MAX_WAIT;
 	INIT_WORK(&q->reset_work, apq_reset_check);
+	vfio_ap_mdev_reset_queue(q);
+	flush_work(&q->reset_work);
 
 	if (matrix_mdev) {
 		vfio_ap_mdev_link_queue(matrix_mdev, q);
@@ -2759,8 +2761,19 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 		vfio_ap_unlink_queue_fr_mdev(q);
 
 	dev_set_drvdata(&apdev->device, NULL);
-	kfree(q);
 	release_update_locks_for_mdev(matrix_mdev);
+
+	if (q->reset_status.response_code != AP_RESPONSE_NORMAL) {
+		/*
+		 * Loop until zeroization of queue is verified so we don't leak
+		 * the internal state of the queue to the caller.
+		 */
+		q->reset_max_wait = 0;
+		vfio_ap_mdev_reset_queue(q);
+		flush_work(&q->reset_work);
+	}
+
+	kfree(q);
 }
 
 /**
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 9bff666b0b35..914ea5d41d33 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -139,6 +139,8 @@ struct ap_matrix_mdev {
  *		 that need to be reset
  * @reset_status: the status from the last reset of the queue
  * @reset_work: work to wait for queue reset to complete
+ * @reset_max_wait: the max amount of time to wait for a reset to complete;
+ *		    specifying 0 means wait as long as it takes.
  */
 struct vfio_ap_queue {
 	struct ap_matrix_mdev *matrix_mdev;
@@ -150,6 +152,7 @@ struct vfio_ap_queue {
 	struct list_head reset_qnode;
 	struct ap_queue_status reset_status;
 	struct work_struct reset_work;
+	unsigned int reset_max_wait;
 };
 
 int vfio_ap_mdev_register(void);
-- 
2.53.0