[PATCH v8 5/6] 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 limits the potential hang to the AP_RESET_MAX_WAIT
(2000ms) timeout introduced in the preceding commit, and
prevents leakage of queue state to a guest.

_queue_passable() accepted 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) does. A queue that is not confirmed zeroized must not be
passed through to a guest, as it may contain key material from
a previous guest or host operation.

Since _queue_passable() now rejects queues that are check stopped
or deconfigured, there is no way for a queue bound to the
vfio_ap device driver to pass those through to a guest even
if they are added back to the configuration or the reason they
have check stopped has been fixed. To resolve this issue,
a new on_queue_state_transition callback function is added to
struct ap_driver which is invoked during the AP bus device scan
when a queue device transitions from deconfigured to configured or
check stopped to not check stopped and vice versa. The vfio_ap device
driver provides an implementation that resets and zeroizes the queue when
it transitions to configured or not check stopped and plugs it into the
guest's AP configuration if the reset succeeds.

To fix this, _queue_passable() is limited to returning true
only when reset_status.response_code == AP_RESPONSE_NORMAL.
A new helper, apq_reset_finalize(), is introduced to ensure
q->reset_status correctly reflects the confirmed end state
of the queue. It copies the full TAPQ status word to
q->reset_status and sets q->reset_status.response_code to
AP_RESPONSE_NORMAL only when apq_status_check() returns 0,
confirming zeroization via TAPQ status bit verification.
For -ENODEV (DECONFIGURED or CHECKSTOPPED), the non-zero
response code is left intact, ensuring _queue_passable()
correctly returns false.

Additionally, vfio_ap_mdev_probe_queue() now calls
vfio_ap_mdev_reset_queue() and flush_work() at probe time
to guarantee a clean queue before it can be assigned to
a guest.

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/ap_bus.c          | 51 +++++++++++++++++++++
 drivers/s390/crypto/ap_bus.h          | 29 ++++++++++++
 drivers/s390/crypto/vfio_ap_drv.c     |  1 +
 drivers/s390/crypto/vfio_ap_ops.c     | 64 ++++++++++++++++++++++-----
 drivers/s390/crypto/vfio_ap_private.h |  2 +
 5 files changed, 137 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index d82df5b4e2db..a53cfad3543e 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -1979,6 +1979,28 @@ static inline void notify_scan_complete(void)
 			 __drv_notify_scan_complete);
 }
 
+/* Helper function for notify_config_changed */
+static int __drv_notify_qstate_transitioned(struct device_driver *drv, void *data)
+{
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	struct ap_qstate_transition *qstate_trans = data;
+
+	if (try_module_get(drv->owner)) {
+		if (ap_drv->on_qstate_transition)
+			ap_drv->on_qstate_transition(qstate_trans);
+		module_put(drv->owner);
+	}
+
+	return 0;
+}
+
+/* Notify all drivers about a queue state transition */
+static inline void notify_qstate_transitioned(struct ap_qstate_transition *qstate_trans)
+{
+	bus_for_each_drv(&ap_bus_type, NULL, qstate_trans,
+			 __drv_notify_qstate_transitioned);
+}
+
 /*
  * Helper function for ap_scan_bus().
  * Remove card device and associated queue devices.
@@ -1998,6 +2020,7 @@ static inline void ap_scan_rm_card_dev_and_queue_devs(struct ap_card *ac)
  */
 static inline void ap_scan_domains(struct ap_card *ac)
 {
+	struct ap_qstate_transition qstate_trans;
 	struct ap_tapq_hwinfo hwinfo;
 	bool decfg, chkstop;
 	struct ap_queue *aq;
@@ -2093,6 +2116,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev checkstop on\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to checkstopped.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CHKSTOP_ON;
+			notify_qstate_transitioned(&qstate_trans);
 			/* 'receive' pending messages with -EAGAIN */
 			ap_flush_queue(aq);
 			goto put_dev_and_continue;
@@ -2104,6 +2134,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev checkstop off\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to not checkstopped.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CHKSTOP_OFF;
+			notify_qstate_transitioned(&qstate_trans);
 			goto put_dev_and_continue;
 		}
 		/* config state change */
@@ -2117,6 +2154,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev config off\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to deconfigured.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CONFIG_OFF;
+			notify_qstate_transitioned(&qstate_trans);
 			ap_send_config_uevent(&aq->ap_dev, aq->config);
 			/* 'receive' pending messages with -EAGAIN */
 			ap_flush_queue(aq);
@@ -2129,6 +2173,13 @@ static inline void ap_scan_domains(struct ap_card *ac)
 			spin_unlock_bh(&aq->lock);
 			pr_debug("(%d,%d) queue dev config on\n",
 				 ac->id, dom);
+			/*
+			 * Notify drivers that the queue state has transitioned
+			 * to configured.
+			 */
+			qstate_trans.queue = aq;
+			qstate_trans.new_state = AP_QUEUE_CONFIG_ON;
+			notify_qstate_transitioned(&qstate_trans);
 			ap_send_config_uevent(&aq->ap_dev, aq->config);
 			goto put_dev_and_continue;
 		}
diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index fb4d678336e4..fd2c7be683e3 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -132,6 +132,29 @@ struct ap_message;
  */
 #define AP_DRIVER_FLAG_DEFAULT 0x0001
 
+/**
+ * ap_queue_state_transition:
+ *
+ * Used to notify a device driver that a queue state transition has occurred.
+ *
+ * @queue:	the queue device whose state transitioned
+ * @new_state:  identifies the new state to which the queue transitioned:
+ *	AP_QUEUE_CONFIG_ON:		from deconfigured to configured
+ *	AP_QUEUE_CONFIG_OFF:		from configured to deconfigured
+ *	AP_QUEUE_CHKSTOPPED_ON:		from not checkstopped to checkstopped
+ *	AP_QUEUE_CHKSTOPPED_OFF:	from checkstopped to not checkstopped
+ */
+struct ap_qstate_transition {
+	struct ap_queue *queue;
+
+	enum {
+		AP_QUEUE_CONFIG_ON,
+		AP_QUEUE_CONFIG_OFF,
+		AP_QUEUE_CHKSTOP_ON,
+		AP_QUEUE_CHKSTOP_OFF,
+	} new_state;
+};
+
 struct ap_driver {
 	struct device_driver driver;
 
@@ -155,6 +178,12 @@ struct ap_driver {
 	void (*on_scan_complete)(struct ap_config_info *new_config_info,
 				 struct ap_config_info *old_config_info);
 
+	/*
+	 * Called during the ap bus scan when a queue state transition is
+	 * detected.
+	 */
+	void (*on_qstate_transition)(struct ap_qstate_transition *qstate_trans);
+
 	struct ap_device_id *ids;
 	unsigned int flags;
 };
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 8e69ed286bb9..6a5d97fa9200 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -61,6 +61,7 @@ static struct ap_driver vfio_ap_drv = {
 	.in_use = vfio_ap_mdev_resource_in_use,
 	.on_config_changed = vfio_ap_on_cfg_changed,
 	.on_scan_complete = vfio_ap_on_scan_complete,
+	.on_qstate_transition = vfio_ap_on_qstate_transition,
 	.ids = ap_queue_ids,
 };
 
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index ffc2d8715bd9..cd4a436c4319 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -841,14 +841,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;
 }
 
 /*
@@ -2809,8 +2809,9 @@ 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));
 	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);
@@ -2902,8 +2903,8 @@ 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);
+	kfree(q);
 }
 
 /**
@@ -3309,3 +3310,46 @@ void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
 
 	mutex_unlock(&matrix_dev->guests_lock);
 }
+
+/**
+ * vfio_ap_on_qstate_transition:
+ *
+ * AP bus callback notifying the vfio_ap device driver that the state of a
+ * queue has transitioned.
+ *
+ * @qstate_trans: the object containing a reference to the queue device and the
+ *		  state to which it transitioned.
+ */
+void vfio_ap_on_qstate_transition(struct ap_qstate_transition *qstate_trans)
+{
+	struct vfio_ap_queue *q = vfio_ap_find_queue(qstate_trans->queue->qid);
+	DECLARE_BITMAP(apm_filtered, AP_DEVICES);
+
+	/*
+	 * If the queue is not bound to the vfio_ap device driver, then it won't
+	 * be passed through to a guest; so, no need to continue.
+	 */
+	if (!q)
+		return;
+
+	get_update_locks_for_mdev(q->matrix_mdev);
+
+	switch (qstate_trans->new_state) {
+	case AP_QUEUE_CONFIG_ON:
+	case AP_QUEUE_CHKSTOP_OFF:
+		vfio_ap_mdev_reset_queue(q);
+		flush_work(&q->reset_work);
+
+		if (q->matrix_mdev) {
+			if (vfio_ap_mdev_filter_matrix(q->matrix_mdev, apm_filtered)) {
+				vfio_ap_mdev_update_guest_apcb(q->matrix_mdev);
+				reset_queues_for_apids(q->matrix_mdev, apm_filtered);
+			}
+		}
+		break;
+	default:
+		break;
+	}
+
+	release_update_locks_for_mdev(q->matrix_mdev);
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 9bff666b0b35..c8b00465d258 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -165,4 +165,6 @@ void vfio_ap_on_cfg_changed(struct ap_config_info *new_config_info,
 void vfio_ap_on_scan_complete(struct ap_config_info *new_config_info,
 			      struct ap_config_info *old_config_info);
 
+void vfio_ap_on_qstate_transition(struct ap_qstate_transition *qstate_trans);
+
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.53.0