RE: [RFC PATCH 06/15] virt: tdx-guest: Support TDI bind and unbind operations

[Duan, Zhenzhong]

>>>>> @@ -181,6 +253,9 @@ static void tdx_devsec_unlock(struct pci_tsm *tsm)
>>>>>   {
>>>>>   	struct tdx_devsec *tdevsec = to_tdx_devsec(tsm);
>>>>>
>>>>> +	if (WARN_ON(tdx_tdi_unbind_dev(tdevsec)))
>>>>> +		return;
>>>> <Thinking out loud>
>>>>
>>>> So unlock involves calling TDCM_UNBIND which in turn might fail for any
>>>> number of reasons and yet we have no way of communicating that. So
>after
>>>> an unlock a TD might want to rebind and issue bind on a device which is
>>>> already bound because prior unbind failed. Is this something we should
>>>> be worried about ?
>>> I completely agree with your point here.
>>> We need pci_tsm_ops::unlock() in TSM framework to return an error status
>>> (e.g., int) so that failures in the underlying hardware-level unbind can
>>> be bubbled up to the caller rather than silently ignored.
>>
>>please remember raising this when folks are discussing generic guest TSM
>>framework series.
>
>Sure.

I want to discuss this further.

If the TSM driver successfully locks a device but later fails to unlock it,
the core cannot confirm that the device has returned to a safe, unlocked
state. The device may still be locked or partially torn down. So besides
return failure to the core, I think the core should mark that device as
broken and prevent it from being attached to a driver. Further lock/unlock/run
should be forbidden in the core, Example changes:

diff --git a/include/linux/pci.h b/include/linux/pci.h
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@
 #ifdef CONFIG_PCI_TSM
     struct pci_tsm *tsm;		/* TSM operation state */
+	bool tsm_broken;			/* Security teardown failed */
 #endif

diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
--- a/include/linux/pci-tsm.h
+++ b/include/linux/pci-tsm.h
@@
-	 * @unlock: destroy TSM context and return device to UNLOCKED state
+	 * @unlock: return device to UNLOCKED state and destroy TSM context.
+	 *	    Return zero on success or a negative errno on failure.
+	 *	    On failure, retain the context for subsequent cleanup.
@@
-		void (*unlock)(struct pci_tsm *tsm);
+		int (*unlock)(struct pci_tsm *tsm);
@@
  * This structure is wrapped by low level TSM driver data and returned by
- * probe()/lock(), it is freed by the corresponding remove()/unlock().
+ * probe()/lock(), it is freed by remove() or a successful unlock().
@@
 struct pci_tsm {
     struct pci_dev *pdev;
     struct pci_dev *dsm_dev;
     struct tsm_dev *tsm_dev;
     struct pci_tdi *tdi;
     struct device_evidence *evidence;
     unsigned long flags;
 };
 
+static inline bool pci_tsm_is_broken(struct pci_dev *pdev)
+{
+#ifdef CONFIG_PCI_TSM
+	return pdev->tsm_broken;
+#else
+	return false;
+#endif
+}
+
diff --git a/drivers/pci/tsm/core.c b/drivers/pci/tsm/core.c
--- a/drivers/pci/tsm/core.c
+++ b/drivers/pci/tsm/core.c
@@
 static int pci_tsm_accept(struct pci_dev *pdev)
 {
@@
     if (pdev->dev.driver)
         return -EBUSY;
 
+	if (pci_tsm_is_broken(pdev))
+		return -EIO;
+
     if (test_bit(PCI_TSM_F_ACCEPT, &pdev->tsm->flags))
         return 0;
@@
 /**
  * pci_tsm_unlock() - Transition TDI from LOCKED/RUN to UNLOCKED
  * @pdev: TDI device to unlock
  *
- * Returns void, requires all callers to have satisfied dependencies like making
- * sure the device is locked and detached from its driver.
+ * Requires the device to be locked and detached from its driver.
+ *
+ * Return: zero on success or a negative errno on failure. An unlock failure
+ * quarantines the device and preserves its TSM context.
  */
-static void pci_tsm_unlock(struct pci_dev *pdev)
+static int pci_tsm_unlock(struct pci_dev *pdev)
 {
+	struct pci_tsm *tsm = pdev->tsm;
+	int ret;
+
     lockdep_assert_held_write(&pci_tsm_rwsem);
     device_lock_assert(&pdev->dev);
 
     if (dev_WARN_ONCE(&pdev->dev, pdev->dev.driver,
               "unlock attempted on driver attached device\n"))
-		return;
+		return -EBUSY;
 
-	to_pci_tsm_ops(pdev->tsm)->unlock(pdev->tsm);
+	ret = to_pci_tsm_ops(tsm)->unlock(tsm);
+	if (ret) {
+		pdev->tsm_broken = true;
+		pci_err(pdev,
+			"TSM unlock failed (%d); device quarantined\n", ret);
+		return ret;
+	}
+
     pdev->tsm = NULL;
     /* implicit clear of PCI_TSM_F_ACCEPT */
+	return 0;
 }
@@
 static int pci_tsm_lock(struct pci_dev *pdev, struct tsm_dev *tsm_dev)
 {
@@
     if (pdev->dev.driver)
         return -EBUSY;
 
+	if (pci_tsm_is_broken(pdev))
+		return -EIO;
+
     tsm = ops->lock(tsm_dev, pdev);
@@
 static ssize_t unlock_store(struct device *dev, struct device_attribute *attr,
               const char *buf, size_t len)
 {
@@
-	pci_tsm_unlock(pdev);
+	rc = pci_tsm_unlock(pdev);
+	if (rc)
+		return rc;
 
     return len;
 }
@@
 int pci_tsm_enable_dma(struct pci_dev *pdev) __must_hold(&pdev->dev->mutex)
 {
@@
     if (dev_WARN_ONCE(dev, !dev->driver,
               "DMA access only finalized at driver attach\n"))
         return -ENXIO;
 
+	if (pci_tsm_is_broken(pdev))
+		return -EIO;
+
     if (!pdev->tsm)
         return 0;

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@
 static int pci_device_probe(struct device *dev)
 {
     int error;
     struct pci_dev *pci_dev = to_pci_dev(dev);
     struct pci_driver *drv = to_pci_driver(dev->driver);
 
+	if (pci_tsm_is_broken(pci_dev)) {
+		pci_err(pci_dev,
+			"Driver probe blocked after TSM unlock failure\n");
+		return -EIO;
+	}
+
     if (!pci_device_can_probe(pci_dev))
         return -ENODEV;

diff --git a/drivers/virt/coco/tdx-guest/connect.c b/drivers/virt/coco/tdx-guest/connect.c
--- a/drivers/virt/coco/tdx-guest/connect.c
+++ b/drivers/virt/coco/tdx-guest/connect.c
@@
-static void tdx_devsec_unlock(struct pci_tsm *tsm)
+static int tdx_devsec_unlock(struct pci_tsm *tsm)
 {
     struct tdx_devsec *tdevsec = to_tdx_devsec(tsm);
     struct device_evidence *evidence = tsm->evidence;
     int ret;
 
     ret = tdx_tdi_unbind_dev(tdevsec);
-	if (ret) {
-		pr_warn("Failed to unbind TDI for %s: %d\n",
-			pci_name(tsm->pdev), ret);
-		return;
-	}
+	if (ret)
+		return ret;
 
     tdx_tdi_mmio_teardown(tdevsec);
 
     dma_set_cc_private(&tsm->pdev->dev, false);
     kfree(evidence->obj[DEVICE_EVIDENCE_TYPE_REPORT].data);
     kfree(evidence);
     kfree(tdevsec);
+	return 0;
 }