iommufd: vIOMMUs and TSM guest requests for confidential guests

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommufd: UAF of kvm_file due to iommufd_device outliving VFIO unbind --

commit ddde31c06624779bad69fb77ae7b9503b553dacc
Author: Shameer Kolothum <[email protected]>

iommufd/device: Associate KVM file pointer with iommufd_device

TSM vDevice support needs access to the KVM associated with a VFIO device after the device has been bound to iommufd. Extend iommufd_device_bind() to accept the device's KVM file and store it in the iommufd_device.

Quoted 4 lines

drivers/iommu/iommufd/device.c +0 -0

index a664c70a6fe73..f99f7bee2e774 100644

[ ... ]

Quoted 12 lines

drivers/iommu/iommufd/device.c +3 -1

@@ -308,7 +309,9 @@ static int iommufd_bind_noiommu(struct iommufd_device *idev)
  * The caller must undo this with iommufd_device_unbind()
  */
 struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
-					   struct device *dev, u32 *id)
+					   struct device *dev,
+					   struct file *kvm_file,
+					   u32 *id)
 {
 	struct iommufd_device *idev;
 	int rc;

drivers/iommu/iommufd/device.c struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx, · line 326

Quoted 5 lines

drivers/iommu/iommufd/device.c +2 -0

@@ -319,6 +322,8 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 
 	idev->ictx = ictx;
 	idev->dev = dev;
+	/* VFIO holds the file reference while the device is open. */
+	idev->kvm_file = kvm_file;

[Severity: High]
Does this code introduce a use-after-free regression for kvm_file when a concurrent ioctl is accessing the device?

If an untrusted user thread executes an ioctl that calls iommufd_get_device(), it will increment idev->obj.users to 2.

If another thread concurrently closes the VFIO device fd, it calls vfio_df_unbind_iommufd() which leads to iommufd_device_unbind() and then iommufd_object_destroy_user().

Because users is 2, iommufd_object_remove() will return -EBUSY:

drivers/iommu/iommufd/main.c:iommufd_object_remove() {
        ...
        if (!refcount_dec_if_one(&obj->users)) {
                ret = -EBUSY;
                goto err_xa;
        }
        ...
}

The unbind operation ignores this error, leaving the idev alive:

drivers/iommu/iommufd/iommufd_private.h:iommufd_object_destroy_user() {
        ...
        ret = iommufd_object_remove(ictx, obj, obj->id, REMOVE_WAIT);
        WARN_ON(ret);
}

The VFIO device then drops its KVM reference during the close process, freeing the kvm_file. When the first thread resumes, will it access the freed kvm_file stored here?

Quoted 58 lines · [email protected] writes:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iommufd: UAF of kvm_file due to iommufd_device outliving VFIO unbind --

commit ddde31c06624779bad69fb77ae7b9503b553dacc
Author: Shameer Kolothum <[email protected]>

iommufd/device: Associate KVM file pointer with iommufd_device

TSM vDevice support needs access to the KVM associated with a VFIO device after the device has been bound to iommufd. Extend iommufd_device_bind() to accept the device's KVM file and store it in the iommufd_device.

Quoted 4 lines

drivers/iommu/iommufd/device.c +0 -0

index a664c70a6fe73..f99f7bee2e774 100644

[ ... ]

Quoted 12 lines

drivers/iommu/iommufd/device.c +3 -1

@@ -308,7 +309,9 @@ static int iommufd_bind_noiommu(struct iommufd_device *idev)
  * The caller must undo this with iommufd_device_unbind()
  */
 struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
-					   struct device *dev, u32 *id)
+					   struct device *dev,
+					   struct file *kvm_file,
+					   u32 *id)
 {
 	struct iommufd_device *idev;
 	int rc;

drivers/iommu/iommufd/device.c struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx, · line 326

Quoted 5 lines

drivers/iommu/iommufd/device.c +2 -0

@@ -319,6 +322,8 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 
 	idev->ictx = ictx;
 	idev->dev = dev;
+	/* VFIO holds the file reference while the device is open. */
+	idev->kvm_file = kvm_file;

[Severity: High]
Does this code introduce a use-after-free regression for kvm_file when a concurrent ioctl is accessing the device?

If an untrusted user thread executes an ioctl that calls iommufd_get_device(), it will increment idev->obj.users to 2.

If another thread concurrently closes the VFIO device fd, it calls vfio_df_unbind_iommufd() which leads to iommufd_device_unbind() and then iommufd_object_destroy_user().

Because users is 2, iommufd_object_remove() will return -EBUSY:

drivers/iommu/iommufd/main.c:iommufd_object_remove() {
        ...
        if (!refcount_dec_if_one(&obj->users)) {
                ret = -EBUSY;
                goto err_xa;
        }
        ...
}

Quoted 6 lines

The unbind operation ignores this error, leaving the idev alive:

drivers/iommu/iommufd/iommufd_private.h:iommufd_object_destroy_user() {
        ...
        ret = iommufd_object_remove(ictx, obj, obj->id, REMOVE_WAIT);
        WARN_ON(ret);

From: Nicolin Chen <[email protected]>

The TSM vDevice operations need access to the KVM associated with the device's vIOMMU. Save the device's KVM file in the iommufd_viommu when the vIOMMU is allocated, and take a file reference so it remains valid for the lifetime of the vIOMMU.

Release the reference when the vIOMMU is destroyed.

Based on an original patch by
Shameer Kolothum <[email protected]>

[nicolinc: hold kvm's users_count]

Signed-off-by Nicolin Chen <[email protected]>

[aneesh.kumar: Switch to use kvm_file]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 drivers/iommu/iommufd/viommu.c | 5 +++++
 include/linux/iommufd.h        | 1 +
 2 files changed, 6 insertions(+)

(quoted diff) +0 -0

b/drivers/iommu/iommufd/viommu.c index f7951057a1e5..b7489ed259bd 100644 --- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c

drivers/iommu/iommufd/viommu.c +3 -0

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
  */
+#include <linux/file.h>
 #include "iommufd_private.h"
 
 void iommufd_viommu_destroy(struct iommufd_object *obj)

@@ -11,6 +12,8 @@ void iommufd_viommu_destroy(struct iommufd_object
 *obj) if (viommu->ops && viommu->ops->destroy)
 		viommu->ops->destroy(viommu);
 	refcount_dec(&viommu->hwpt->common.obj.users);
+	if (viommu->kvm_file)
+		fput(viommu->kvm_file);
 	xa_destroy(&viommu->vdevs);
 }
 

drivers/iommu/iommufd/viommu.c +0 -0

 	viommu->type = cmd->type;
 	viommu->ictx = ucmd->ictx;
 	viommu->hwpt = hwpt_paging;

include/linux/iommufd.h +1 -0

index 0a0bb4abfbd2..3267717f676d 100644
@@ -103,6 +103,7 @@ struct iommufd_viommu {
 	struct iommufd_ctx *ictx;
 	struct iommu_device *iommu_dev;
 	struct iommufd_hwpt_paging *hwpt;
+	struct file *kvm_file;
 
 	const struct iommufd_viommu_ops *ops;
 

Hi Aneesh,

Quoted 48 lines · On Thu, 8 Oct 2026 11:29:47 +0530 "Aneesh Kumar K.V (Arm)"…

From: Nicolin Chen <[email protected]>

The TSM vDevice operations need access to the KVM associated with the device's vIOMMU. Save the device's KVM file in the iommufd_viommu when the vIOMMU is allocated, and take a file reference so it remains valid for the lifetime of the vIOMMU.

Release the reference when the vIOMMU is destroyed.

Based on an original patch by
Shameer Kolothum <[email protected]>

[nicolinc: hold kvm's users_count]

Signed-off-by Nicolin Chen <[email protected]>

[aneesh.kumar: Switch to use kvm_file]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 drivers/iommu/iommufd/viommu.c | 5 +++++
 include/linux/iommufd.h        | 1 +
 2 files changed, 6 insertions(+)

(quoted diff) +0 -0

b/drivers/iommu/iommufd/viommu.c index f7951057a1e5..b7489ed259bd 100644 --- a/drivers/iommu/iommufd/viommu.c
+++ b/drivers/iommu/iommufd/viommu.c

drivers/iommu/iommufd/viommu.c +3 -0

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
  */
+#include <linux/file.h>
 #include "iommufd_private.h"
 
 void iommufd_viommu_destroy(struct iommufd_object *obj)

@@ -11,6 +12,8 @@ void iommufd_viommu_destroy(struct iommufd_object
 *obj) if (viommu->ops && viommu->ops->destroy)
 		viommu->ops->destroy(viommu);
 	refcount_dec(&viommu->hwpt->common.obj.users);
+	if (viommu->kvm_file)
+		fput(viommu->kvm_file);
 	xa_destroy(&viommu->vdevs);
 }
 

drivers/iommu/iommufd/viommu.c int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd · line 89

Quoted 5 lines

drivers/iommu/iommufd/viommu.c +2 -0

@@ -82,6 +85,8 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd
 *ucmd) }
 
 	xa_init(&viommu->vdevs);
+	if (idev->kvm_file)
+		viommu->kvm_file = get_file(idev->kvm_file);

So kvm_file will persist inside viommu even when no more vdevice exists under the viommu. In that case, don't we need to check idev->kvm_file matches viommu->kvm_file during vdevice allocation?

Otherwise, it appears possible to associate a device belonging to a different KVM VM with the existing vIOMMU, no?

From: Aneesh Kumar K.V <[email protected]>
Sent: Thursday, October 29, 2026 2:26 PM

Quoted 6 lines · Jacob Pan

Hi Aneesh,

Review int iommufd_viommu_alloc_ioctl(struct

Quoted 1 line · On Thu, 8 Oct 2026 11:29:47 +0530 "Aneesh Kumar K.V (Arm)"…

(quoted diff) +0 -0

@@ -82,6 +85,8 @@ int iommufd_viommu_alloc_ioctl(struct

iommufd_ucmd

Quoted 9 lines

drivers/iommu/iommufd/viommu.c int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd) · line 89

Quoted 5 lines

drivers/iommu/iommufd/viommu.c +2 -0

 *ucmd) }
 
 	xa_init(&viommu->vdevs);
+	if (idev->kvm_file)
+		viommu->kvm_file = get_file(idev->kvm_file);

So kvm_file will persist inside viommu even when no more vdevice exists under the viommu. In that case, don't we need to check idev->kvm_file matches viommu->kvm_file during vdevice allocation?

Quoted 3 lines

Otherwise, it appears possible to associate a device belonging to a different KVM VM with the existing vIOMMU, no?

Agreed. I will add the below.

modified drivers/iommu/iommufd/viommu.c

Besides, it might be a good idea to return an error if idev->kvm_file is NULL in iommufd_viommu_alloc_ioctl(). I don't think the viommu logic is designed for native application...

The TSM vDevice operations need access to the KVM associated with the device's vIOMMU. Save the device's KVM file in the iommufd_viommu when the vIOMMU is allocated, and take a file reference so it remains valid for the lifetime of the vIOMMU.

Release the reference when the vIOMMU is destroyed.

[...]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tsm: race in tsm_register() error path leads to UAF and NULL pointer deref --

commit f9b77b7469552ae6078c3a47333908a88eec2474
Author: Aneesh Kumar K.V (Arm) <[email protected]>

tsm: Remove the device from lookup before PCI teardown

This commit modifies the teardown flow to remove the TSM device from lookup before tearing down the PCI state. It also adjusts the setup flow in tsm_register() to handle PCI registration failures differently.

Quoted 4 lines

drivers/virt/coco/tsm-core.c +0 -0

@@ -56,26 +56,25 @@ static struct tsm_dev *alloc_tsm_dev(struct device *parent)

[ ... ]

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -2

-static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
-						 struct pci_tsm_ops *pci_ops)
+static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)
 {

[ ... ]

drivers/virt/coco/tsm-core.c static struct tsm_dev *alloc_tsm_dev(struct device *parent) · line 69

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -0

 	tsm_dev->pci_ops = pci_ops;
 	rc = pci_tsm_register(tsm_dev);
 	if (rc) {
+		tsm_dev->pci_ops = NULL;

[Severity: Medium]
Does this unprotected write to tsm_dev->pci_ops introduce a race condition?

If pci_tsm_register() returns an error early (e.g. if a driver incorrectly provides both link_ops and devsec_ops, returning -EINVAL before acquiring pci_tsm_rwsem), a concurrent userspace write to the connect sysfs attribute might trigger connect_store().

If connect_store() evaluates is_link_tsm() concurrently, this lockless assignment to NULL on the error path could cause the compiler to reload the pointer after the NULL check. Could this lead to a NULL pointer dereference when accessing ->link_ops.probe?

Quoted 6 lines

drivers/virt/coco/tsm-core.c +1 -2

 		dev_err(tsm_dev->dev.parent,
 			"PCI/TSM registration failure: %d\n", rc);
-		device_unregister(&tsm_dev->dev);
-		return ERR_PTR(rc);
+		return rc;
 	}

[ ... ]

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 100

Quoted 8 lines

drivers/virt/coco/tsm-core.c +3 -1

@@ -96,15 +95,22 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
 	if (rc)
 		return ERR_PTR(rc);
 
-	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);
+	rc = tsm_register_pci(tsm_dev, pci_ops);
+	if (rc) {
+		device_del(dev);

[Severity: Medium]
Can this error path lead to a use-after-free if PCI registration fails?

Since device_add() is called prior to tsm_register_pci() in tsm_register(), the device is already exposed via class_find_device(). If a concurrent connect_store() finds the device and proceeds to pci_tsm_connect() before this error path cleans it up, it will attach the PCI context to tsm_dev.

When this error path calls device_del() and returns, the __free(put_tsm_dev) guard at the top of tsm_register() will drop the primary reference to tsm_dev, freeing its memory.

Because the attached PCI context stores the pointer without taking an additional device reference, wouldn't this leave a dangling pointer in the PCI context, resulting in a use-after-free on subsequent accesses?

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 104

Quoted 4 lines

drivers/virt/coco/tsm-core.c +3 -0

+		return ERR_PTR(rc);
+	}
+	return no_free_ptr(tsm_dev);
 }

Quoted 73 lines · [email protected] writes:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tsm: race in tsm_register() error path leads to UAF and NULL pointer deref --

commit f9b77b7469552ae6078c3a47333908a88eec2474
Author: Aneesh Kumar K.V (Arm) <[email protected]>

tsm: Remove the device from lookup before PCI teardown

This commit modifies the teardown flow to remove the TSM device from lookup before tearing down the PCI state. It also adjusts the setup flow in tsm_register() to handle PCI registration failures differently.

Quoted 4 lines

drivers/virt/coco/tsm-core.c +0 -0

@@ -56,26 +56,25 @@ static struct tsm_dev *alloc_tsm_dev(struct device *parent)

[ ... ]

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -2

-static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
-						 struct pci_tsm_ops *pci_ops)
+static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)
 {

[ ... ]

drivers/virt/coco/tsm-core.c static struct tsm_dev *alloc_tsm_dev(struct device *parent) · line 69

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -0

 	tsm_dev->pci_ops = pci_ops;
 	rc = pci_tsm_register(tsm_dev);
 	if (rc) {
+		tsm_dev->pci_ops = NULL;

[Severity: Medium]
Does this unprotected write to tsm_dev->pci_ops introduce a race condition?

If pci_tsm_register() returns an error early (e.g. if a driver incorrectly provides both link_ops and devsec_ops, returning -EINVAL before acquiring pci_tsm_rwsem), a concurrent userspace write to the connect sysfs attribute might trigger connect_store().

If connect_store() evaluates is_link_tsm() concurrently, this lockless assignment to NULL on the error path could cause the compiler to reload the pointer after the NULL check. Could this lead to a NULL pointer dereference when accessing ->link_ops.probe?

Quoted 6 lines

drivers/virt/coco/tsm-core.c +1 -2

 		dev_err(tsm_dev->dev.parent,
 			"PCI/TSM registration failure: %d\n", rc);
-		device_unregister(&tsm_dev->dev);
-		return ERR_PTR(rc);
+		return rc;
 	}

[ ... ]

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 100

Quoted 8 lines

drivers/virt/coco/tsm-core.c +3 -1

@@ -96,15 +95,22 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
 	if (rc)
 		return ERR_PTR(rc);
 
-	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);
+	rc = tsm_register_pci(tsm_dev, pci_ops);
+	if (rc) {
+		device_del(dev);

[Severity: Medium]
Can this error path lead to a use-after-free if PCI registration fails?

Since device_add() is called prior to tsm_register_pci() in tsm_register(), the device is already exposed via class_find_device(). If a concurrent connect_store() finds the device and proceeds to pci_tsm_connect() before this error path cleans it up, it will attach the PCI context to tsm_dev.

When this error path calls device_del() and returns, the __free(put_tsm_dev) guard at the top of tsm_register() will drop the primary reference to tsm_dev, freeing its memory.

Because the attached PCI context stores the pointer without taking an additional device reference, wouldn't this leave a dangling pointer in the PCI context, resulting in a use-after-free on subsequent accesses?

Quoted 4 lines

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 104

Quoted 4 lines

drivers/virt/coco/tsm-core.c +3 -0

+		return ERR_PTR(rc);
+	}
+	return no_free_ptr(tsm_dev);
 }

I think this problem already exists upstream. We should delay device_add() until PCI/TSM registration succeeds. The kobject event should also be emitted after device_add(). I will add the fix as a separate patch.

Quoted 75 lines · Aneesh Kumar K.V

Quoted 73 lines · [email protected] writes:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tsm: race in tsm_register() error path leads to UAF and NULL pointer deref --

commit f9b77b7469552ae6078c3a47333908a88eec2474
Author: Aneesh Kumar K.V (Arm) <[email protected]>

tsm: Remove the device from lookup before PCI teardown

This commit modifies the teardown flow to remove the TSM device from lookup before tearing down the PCI state. It also adjusts the setup flow in tsm_register() to handle PCI registration failures differently.

Quoted 4 lines

drivers/virt/coco/tsm-core.c +0 -0

@@ -56,26 +56,25 @@ static struct tsm_dev *alloc_tsm_dev(struct device *parent)

[ ... ]

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -2

-static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
-						 struct pci_tsm_ops *pci_ops)
+static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)
 {

[ ... ]

drivers/virt/coco/tsm-core.c static struct tsm_dev *alloc_tsm_dev(struct device *parent) · line 69

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -0

 	tsm_dev->pci_ops = pci_ops;
 	rc = pci_tsm_register(tsm_dev);
 	if (rc) {
+		tsm_dev->pci_ops = NULL;

[Severity: Medium]
Does this unprotected write to tsm_dev->pci_ops introduce a race condition?

If pci_tsm_register() returns an error early (e.g. if a driver incorrectly provides both link_ops and devsec_ops, returning -EINVAL before acquiring pci_tsm_rwsem), a concurrent userspace write to the connect sysfs attribute might trigger connect_store().

If connect_store() evaluates is_link_tsm() concurrently, this lockless assignment to NULL on the error path could cause the compiler to reload the pointer after the NULL check. Could this lead to a NULL pointer dereference when accessing ->link_ops.probe?

Quoted 6 lines

drivers/virt/coco/tsm-core.c +1 -2

 		dev_err(tsm_dev->dev.parent,
 			"PCI/TSM registration failure: %d\n", rc);
-		device_unregister(&tsm_dev->dev);
-		return ERR_PTR(rc);
+		return rc;
 	}

[ ... ]

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 100

Quoted 8 lines

drivers/virt/coco/tsm-core.c +3 -1

@@ -96,15 +95,22 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
 	if (rc)
 		return ERR_PTR(rc);
 
-	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);
+	rc = tsm_register_pci(tsm_dev, pci_ops);
+	if (rc) {
+		device_del(dev);

[Severity: Medium]
Can this error path lead to a use-after-free if PCI registration fails?

Since device_add() is called prior to tsm_register_pci() in tsm_register(), the device is already exposed via class_find_device(). If a concurrent connect_store() finds the device and proceeds to pci_tsm_connect() before this error path cleans it up, it will attach the PCI context to tsm_dev.

When this error path calls device_del() and returns, the __free(put_tsm_dev) guard at the top of tsm_register() will drop the primary reference to tsm_dev, freeing its memory.

Because the attached PCI context stores the pointer without taking an additional device reference, wouldn't this leave a dangling pointer in the PCI context, resulting in a use-after-free on subsequent accesses?

Quoted 10 lines

Quoted 4 lines

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 104

Quoted 4 lines

drivers/virt/coco/tsm-core.c +3 -0

+		return ERR_PTR(rc);
+	}
+	return no_free_ptr(tsm_dev);
 }

I think this problem already exists upstream. We should delay device_add() until PCI/TSM registration succeeds. The kobject event should also be emitted after device_add(). I will add the fix as a separate patch.

Sigh, trying to do all this stuff through sysfs makes such an mess.. Nothing gets to be atomic in sysfs land so..

It looks like this ordering is to ensure that when connect appears on the pci device it can immediately be used with the tsm that triggered it's creation. I'm not sure this is valuable but I haven't worked out how a userspace is even supposed to exist with all these troublesome races.

If you swap the order and remove the extra events then userspace can see a connect sysfs but no tsm0 device. I think this is possibly better as userspace will want to obtain the tsm handle then process all the devices affiliated with it. So waiting around for a tsm and then knowning that every connect is already there strikes me as easier to work with. IDK

The bigger issue here is I have no idea how the userspace is supposed to work. When my imagined 'systemd device accept d' starts it will see a sytem with no TSM and no connect sysfs.

A tsm may eventually appear when the TSM module gets loaded, eg it loads by udev through the smccc bus on CCA.

Or it might need manual module loading because it is a SW DOE based tsm.

Or it might never ever appear.

So what is the device-accept-d logic supposed to even look like?

Before touching this we need to come up with a draft systemd-device-accept-d shaped thing that does the TSM startup before drivers bind. Then we can see fully how it can work. I asked someone to start working on this, but I expected there to be a ready story on how it should work, not more design work.

One possible answer is more TSM stuff has to be built in so we don't hotplug the default TSM. Then the userspace simply checks for a TSM, if present it connects it, otherwise maybe it thinks about using the DOE one just to get an attestation log.

Module auto loading looks troublesome too, if we want to use the SW based DOE TSM then how does device-accept-d trigger the module load when there isn't even any sysfs?

Once again I'm thinking if the sysfs as a mistake and wondering if it works better using a /dev/tsm.. :(

In any event, lets not get everything tangled into everything else. This series is about viommu, lets keep track of these other issues and keep focusing on viommu..

The proposed fix is to reorder registration so that device_add() is called last.

I expect these operations to require manual setup: the administrator would first unbind the device from its driver, configure IDE, and then bind it to vfio-pci if the device is to be passed through.

Since device bind and unbind operations are already driven through sysfs, it seems natural for the connect operation to use sysfs as well?

Quoted 87 lines · Jason Gunthorpe

Quoted 75 lines · Aneesh Kumar K.V

Quoted 73 lines · [email protected] writes:

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tsm: race in tsm_register() error path leads to UAF and NULL pointer deref --

commit f9b77b7469552ae6078c3a47333908a88eec2474
Author: Aneesh Kumar K.V (Arm) <[email protected]>

tsm: Remove the device from lookup before PCI teardown

This commit modifies the teardown flow to remove the TSM device from lookup before tearing down the PCI state. It also adjusts the setup flow in tsm_register() to handle PCI registration failures differently.

Quoted 4 lines

drivers/virt/coco/tsm-core.c +0 -0

@@ -56,26 +56,25 @@ static struct tsm_dev *alloc_tsm_dev(struct device *parent)

[ ... ]

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -2

-static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
-						 struct pci_tsm_ops *pci_ops)
+static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)
 {

[ ... ]

drivers/virt/coco/tsm-core.c static struct tsm_dev *alloc_tsm_dev(struct device *parent) · line 69

Quoted 4 lines

drivers/virt/coco/tsm-core.c +1 -0

 	tsm_dev->pci_ops = pci_ops;
 	rc = pci_tsm_register(tsm_dev);
 	if (rc) {
+		tsm_dev->pci_ops = NULL;

[Severity: Medium]
Does this unprotected write to tsm_dev->pci_ops introduce a race condition?

If pci_tsm_register() returns an error early (e.g. if a driver incorrectly provides both link_ops and devsec_ops, returning -EINVAL before acquiring pci_tsm_rwsem), a concurrent userspace write to the connect sysfs attribute might trigger connect_store().

If connect_store() evaluates is_link_tsm() concurrently, this lockless assignment to NULL on the error path could cause the compiler to reload the pointer after the NULL check. Could this lead to a NULL pointer dereference when accessing ->link_ops.probe?

Quoted 6 lines

drivers/virt/coco/tsm-core.c +1 -2

 		dev_err(tsm_dev->dev.parent,
 			"PCI/TSM registration failure: %d\n", rc);
-		device_unregister(&tsm_dev->dev);
-		return ERR_PTR(rc);
+		return rc;
 	}

[ ... ]

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 100

Quoted 8 lines

drivers/virt/coco/tsm-core.c +3 -1

@@ -96,15 +95,22 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
 	if (rc)
 		return ERR_PTR(rc);
 
-	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);
+	rc = tsm_register_pci(tsm_dev, pci_ops);
+	if (rc) {
+		device_del(dev);

[Severity: Medium]
Can this error path lead to a use-after-free if PCI registration fails?

Since device_add() is called prior to tsm_register_pci() in tsm_register(), the device is already exposed via class_find_device(). If a concurrent connect_store() finds the device and proceeds to pci_tsm_connect() before this error path cleans it up, it will attach the PCI context to tsm_dev.

When this error path calls device_del() and returns, the __free(put_tsm_dev) guard at the top of tsm_register() will drop the primary reference to tsm_dev, freeing its memory.

Because the attached PCI context stores the pointer without taking an additional device reference, wouldn't this leave a dangling pointer in the PCI context, resulting in a use-after-free on subsequent accesses?

Quoted 10 lines

Quoted 4 lines

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 104

Quoted 4 lines

drivers/virt/coco/tsm-core.c +3 -0

+		return ERR_PTR(rc);
+	}
+	return no_free_ptr(tsm_dev);
 }

I think this problem already exists upstream. We should delay device_add() until PCI/TSM registration succeeds. The kobject event should also be emitted after device_add(). I will add the fix as a separate patch.

Quoted 9 lines

Sigh, trying to do all this stuff through sysfs makes such an mess.. Nothing gets to be atomic in sysfs land so..

It looks like this ordering is to ensure that when connect appears on the pci device it can immediately be used with the tsm that triggered it's creation. I'm not sure this is valuable but I haven't worked out how a userspace is even supposed to exist with all these troublesome races.

The issue is theoretical and does not arise with a single link TSM driver. The PCI device's "connect" attribute is added only after pci_tsm_register() succeeds.

The race can occur when registering a second TSM. The "connect" attribute already exists, and userspace may try to connect using the new link driver, for example "tsm1". connect_store() then looks up ID 1 and can find the new tsm_dev in tsm_class because device_add() has already completed.

The proposed fix is to reorder registration so that device_add() is called last.

ie, changes the sequence from:

	rc = dev_set_name(dev, "tsm%d", tsm_dev->id);
	if (rc)
		return ERR_PTR(rc);

	rc = device_add(dev);
	if (rc)
		return ERR_PTR(rc);

	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);

to this.

	rc = dev_set_name(dev, "tsm%d", tsm_dev->id);
	if (rc)
		return ERR_PTR(rc);

	/* Complete PCI registration before exposing the TSM to class lookup. */
	rc = tsm_register_pci(tsm_dev, pci_ops);

	if (rc)
		return ERR_PTR(rc);

	rc = device_add(dev);
	if (rc) {

		if (pci_ops)
			pci_tsm_unregister(tsm_dev);
		return ERR_PTR(rc);
	}

	/* Notify TSM userspace that PCI/TSM operations are now possible. */
	if (pci_ops)
		kobject_uevent(&dev->kobj, KOBJ_CHANGE);

drivers/virt/coco/tsm-core.c static struct tsm_dev *alloc_tsm_dev(struct device *parent) · line 64

Quoted 10 lines

[ ... 22 lines skipped ... ]

drivers/virt/coco/tsm-core.c +2 -3

-static struct tsm_dev *tsm_register_pci_or_reset(struct tsm_dev *tsm_dev,
-						 struct pci_tsm_ops *pci_ops)
+static int tsm_register_pci(struct tsm_dev *tsm_dev, struct pci_tsm_ops *pci_ops)
 {
 	int rc;
 
 	if (!pci_ops)
-		return tsm_dev;
+		return 0;

Aside unrelated to this series: Do we need this NULL pci_ops stuff? I don't know what it was imagined to do, but now that we have split things up attestation subsystem I wonder if it would ever be useful?

It is quite a maze of unused layers in here..

drivers/virt/coco/tsm-core.c static struct tsm_dev *alloc_tsm_dev(struct device *parent) · line 69

Quoted 6 lines

drivers/virt/coco/tsm-core.c +1 -0

 	tsm_dev->pci_ops = pci_ops;
 	rc = pci_tsm_register(tsm_dev);
 	if (rc) {
+		tsm_dev->pci_ops = NULL;

[Severity: Medium]
Does this unprotected write to tsm_dev->pci_ops introduce a race condition?

And of course sashiko finds races in all this unused complexity...

drivers/virt/coco/tsm-core.c struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops) · line 100

Quoted 9 lines

[ ... 14 lines skipped ... ]

drivers/virt/coco/tsm-core.c +3 -1

@@ -96,15 +95,22 @@ struct tsm_dev *tsm_register(struct device *parent, struct pci_tsm_ops *pci_ops)
 	if (rc)
 		return ERR_PTR(rc);
 
-	return tsm_register_pci_or_reset(no_free_ptr(tsm_dev), pci_ops);
+	rc = tsm_register_pci(tsm_dev, pci_ops);
+	if (rc) {
+		device_del(dev);

[Severity: Medium]
Can this error path lead to a use-after-free if PCI registration fails?

Since device_add() is called prior to tsm_register_pci() in tsm_register(), the device is already exposed via class_find_device(). If a concurrent connect_store() finds the device and proceeds to pci_tsm_connect() before this error path cleans it up, it will attach the PCI context to tsm_dev.

Fun. Usually device_add should be the last thing in any driver flow, not the first. There should not be any device_del on error paths.

Because of races like this...

[ ... 300 lines skipped ... ]

include/uapi/linux/iommufd.h +17 -0

+/**
+ * enum iommu_vdevice_tsm_guest_req_op - Guest TSM request operations
+ * @TSM_REQ_VALIDATE_MMIO: Validate and map a guest MMIO range for the trusted
+ *                         device.
+ * @TSM_REQ_SET_TDI_STATE: Transition the trusted device to the unlocked,
+ *                         locked, or running state.
+ * @TSM_REQ_SEV_ENABLE_DMA: Enable DMA for an SEV device.
+ * @TSM_REQ_SEV_DISABLE_DMA: Disable DMA for an SEV device.
+ * @TSM_REQ_READ_OBJECT: Read bytes from a device object, such as a certificate,
+ *                       measurement, or interface report.
+ * @TSM_REQ_REGEN_OBJECT: Regenerate a device object, such as a measurement or
+ *                        interface report.
+ * @TSM_REQ_OBJECT_INFO: Query the size of a device object.
+ * @TSM_REQ_MAX: One past the last request operation; not a command.
+ *
+ * The request payload and response format depend on the TVM architecture.
+ */

include/uapi/linux/iommufd.h +29 -0

+
+/**
+ * struct iommu_vdevice_tsm_req - ioctl(IOMMU_VDEVICE_TSM_REQ)
+ * @size: sizeof(struct iommu_vdevice_tsm_req)
+ * @vdevice_id: vDevice ID the guest request is for
+ * @op: One of enum iommu_vdevice_tsm_guest_req_op, excluding TSM_REQ_MAX
+ * @tvm_arch: One of enum iommu_vdevice_tsm_guest_tvm_arch
+ * @req_len: Size in bytes of the input payload at @req_uptr, at most INT_MAX
+ * @resp_len: Size in bytes of the output buffer at @resp_uptr, at most INT_MAX
+ * @req_uptr: Userspace pointer to the guest-provided request payload
+ * @resp_uptr: Userspace pointer to the guest response buffer
+ * @out_tsm_code: TSM-specific result code returned by the TSM implementation
+ *
+ * Forward a TSM request to the TSM bound vDevice. This is intended for
+ * guest TSM/TDISP message transport where the host kernel only marshals
+ * bytes between userspace and the TSM implementation.
+ *
+ * The request operation is guest initiated. The vDEVICE implementation
+ * declares the TVM architecture and operations it accepts; iommufd checks
+ * both before forwarding the request.
+ *
+ * The request payload is read from @req_uptr/@req_len. If a response is
+ * expected, userspace provides @resp_uptr/@resp_len as writable storage for
+ * response bytes returned by the TSM path.
+ *
+ * The ioctl is only suitable for commands and results that the host kernel
+ * has no use, the host is only facilitating guest to TSM communication.
+ */
+struct iommu_vdevice_tsm_req {

Quoted 18 lines

[ ... 300 lines skipped ... ]

include/uapi/linux/iommufd.h +17 -0

+/**
+ * enum iommu_vdevice_tsm_guest_req_op - Guest TSM request operations
+ * @TSM_REQ_VALIDATE_MMIO: Validate and map a guest MMIO range for the trusted
+ *                         device.
+ * @TSM_REQ_SET_TDI_STATE: Transition the trusted device to the unlocked,
+ *                         locked, or running state.
+ * @TSM_REQ_SEV_ENABLE_DMA: Enable DMA for an SEV device.
+ * @TSM_REQ_SEV_DISABLE_DMA: Disable DMA for an SEV device.
+ * @TSM_REQ_READ_OBJECT: Read bytes from a device object, such as a certificate,
+ *                       measurement, or interface report.
+ * @TSM_REQ_REGEN_OBJECT: Regenerate a device object, such as a measurement or
+ *                        interface report.
+ * @TSM_REQ_OBJECT_INFO: Query the size of a device object.
+ * @TSM_REQ_MAX: One past the last request operation; not a command.
+ *
+ * The request payload and response format depend on the TVM architecture.
+ */

include/uapi/linux/iommufd.h struct iommu_hw_queue_alloc { · line 1433

Quoted 10 lines

include/uapi/linux/iommufd.h +10 -0

+enum iommu_vdevice_tsm_guest_req_op {
+	TSM_REQ_VALIDATE_MMIO = 1,
+	TSM_REQ_SET_TDI_STATE,
+	TSM_REQ_SEV_ENABLE_DMA,
+	TSM_REQ_SEV_DISABLE_DMA,
+	TSM_REQ_READ_OBJECT,
+	TSM_REQ_REGEN_OBJECT,
+	TSM_REQ_OBJECT_INFO,
+	TSM_REQ_MAX,
+};

This is still no where close to being well defined enough.

Everything here must tie back to defined spec references, listed in the comments.

I cannot guess what data is passed over any of these APIs, how things like "REGEN_OBJECT" select which object to regen, or what the format of those objects even is. The formats of req/resp are never specified.

The comment for this should probably be *enormous*. Every arch should explain what they do on every one of these ops, and it should be identical if it is structured like this.

I have a strong guess almost none of this is actually general, but lets find out.

You can also put enum kdocs directly before each enum which helps alot when they get as big as this stuff has to get.

My guesses with no information
1) VALIDATE_MMIO why would the hypervisor ever need this? Guest validates MMIO through secure communication to the secure world.

2) TDI_STATE I thought at least RMM did this with an RSI?

3) ENABLE/DISABLE DMA, see #1, must not be done by hypervisor See my reply to Alexey:

   https://lore.kernel.org/linux-iommu/[email protected]/

4) READ/REGEN/INFO, what are the objects? What format are they in? How do you select them?

Does this overlap with attestation subsystem? Should attestation subsystem be used to access the SPDM related data?

Update the comment to explain all this, don't need to answer here.

We really need to do these defines along with implementations, that probably will settle alot of the questions above.

Quoted 34 lines

include/uapi/linux/iommufd.h +34 -0

+
+/**
+ * struct iommu_vdevice_tsm_req - ioctl(IOMMU_VDEVICE_TSM_REQ)
+ * @size: sizeof(struct iommu_vdevice_tsm_req)
+ * @vdevice_id: vDevice ID the guest request is for
+ * @op: One of enum iommu_vdevice_tsm_guest_req_op, excluding TSM_REQ_MAX
+ * @tvm_arch: One of enum iommu_vdevice_tsm_guest_tvm_arch
+ * @req_len: Size in bytes of the input payload at @req_uptr, at most INT_MAX
+ * @resp_len: Size in bytes of the output buffer at @resp_uptr, at most INT_MAX
+ * @req_uptr: Userspace pointer to the guest-provided request payload
+ * @resp_uptr: Userspace pointer to the guest response buffer
+ * @out_tsm_code: TSM-specific result code returned by the TSM implementation
+ *
+ * Forward a TSM request to the TSM bound vDevice. This is intended for
+ * guest TSM/TDISP message transport where the host kernel only marshals
+ * bytes between userspace and the TSM implementation.
+ *
+ * The request operation is guest initiated. The vDEVICE implementation
+ * declares the TVM architecture and operations it accepts; iommufd checks
+ * both before forwarding the request.
+ *
+ * The request payload is read from @req_uptr/@req_len. If a response is
+ * expected, userspace provides @resp_uptr/@resp_len as writable storage for
+ * response bytes returned by the TSM path.
+ *
+ * The ioctl is only suitable for commands and results that the host kernel
+ * has no use, the host is only facilitating guest to TSM communication.
+ */
+struct iommu_vdevice_tsm_req {
+	__u32 size;
+	__u32 vdevice_id;
+	__u32 op;
+	__u32 tvm_arch;
+	__u32 req_len;

include/uapi/linux/iommufd.h struct iommu_hw_queue_alloc { · line 1472

Quoted 5 lines

include/uapi/linux/iommufd.h +5 -0

+	__u32 resp_len;
+	__aligned_u64 req_uptr;
+	__aligned_u64 resp_uptr;
+	__aligned_u64 out_tsm_code;
+};

Should the viommu_id be passed in as well, or we know it implicitly from the vdevice_id?

I think this is probably a fine generic API, it should probably not be "TSM request" so that the other viommu people can reuse the transport.

I would probably try to stick to exactly like the other APIs do with a single enum. Two enums for op/arch is saying already that the ops are not well defined.

So maybe strip this patch out to have no tsm stuff, define no ops and just give a command transport into the viommu driver?

Quoted 31 lines · Jason Gunthorpe

Quoted 18 lines

[ ... 300 lines skipped ... ]

include/uapi/linux/iommufd.h +17 -0

+/**
+ * enum iommu_vdevice_tsm_guest_req_op - Guest TSM request operations
+ * @TSM_REQ_VALIDATE_MMIO: Validate and map a guest MMIO range for the trusted
+ *                         device.
+ * @TSM_REQ_SET_TDI_STATE: Transition the trusted device to the unlocked,
+ *                         locked, or running state.
+ * @TSM_REQ_SEV_ENABLE_DMA: Enable DMA for an SEV device.
+ * @TSM_REQ_SEV_DISABLE_DMA: Disable DMA for an SEV device.
+ * @TSM_REQ_READ_OBJECT: Read bytes from a device object, such as a certificate,
+ *                       measurement, or interface report.
+ * @TSM_REQ_REGEN_OBJECT: Regenerate a device object, such as a measurement or
+ *                        interface report.
+ * @TSM_REQ_OBJECT_INFO: Query the size of a device object.
+ * @TSM_REQ_MAX: One past the last request operation; not a command.
+ *
+ * The request payload and response format depend on the TVM architecture.
+ */

include/uapi/linux/iommufd.h struct iommu_hw_queue_alloc { · line 1433

Quoted 10 lines

include/uapi/linux/iommufd.h +10 -0

+enum iommu_vdevice_tsm_guest_req_op {
+	TSM_REQ_VALIDATE_MMIO = 1,
+	TSM_REQ_SET_TDI_STATE,
+	TSM_REQ_SEV_ENABLE_DMA,
+	TSM_REQ_SEV_DISABLE_DMA,
+	TSM_REQ_READ_OBJECT,
+	TSM_REQ_REGEN_OBJECT,
+	TSM_REQ_OBJECT_INFO,
+	TSM_REQ_MAX,
+};

This is still no where close to being well defined enough.

Quoted 7 lines

Everything here must tie back to defined spec references, listed in the comments.

I cannot guess what data is passed over any of these APIs, how things like "REGEN_OBJECT" select which object to regen, or what the format of those objects even is. The formats of req/resp are never specified.

The request and response payload formats are specific to each CoCo backend. Since iommufd treats the payloads as opaque and does not validate their contents, what would documenting those formats here achieve?

The limited validation is intended to prevent this from becoming a general-purpose multiplexer ioctl.

It restricts the ioctl to a defined set of guest-initiated operations that the host forwards to the firmware. The path is essentially:

guest -> vmexit -> VMM -> iommufd ioctl -> TSM backend -> firmware

Quoted 4 lines

The comment for this should probably be *enormous*. Every arch should explain what they do on every one of these ops, and it should be identical if it is structured like this.

That would require documenting a significant amount of backend-specific detail in iommufd. Could we instead require each TSM backend's guest_request callback to document its request and response formats?

Quoted 4 lines

My guesses with no information
1) VALIDATE_MMIO why would the hypervisor ever need this? Guest validates MMIO through secure communication to the secure world.

With VFIO PCI BARs backed by guest_memfd, the VMM can validate that the IPA and PA values provided by the guest belong to the VFIO device.

We previously also used this operation to ask the CoCo TSM backend to map the device memory (the BAR regions). I have removed this operation from the latest series because that responsibility is now split between the VMM and guest_memfd. TDX and SNP still need to be updated accordingly.

Quoted 1 line

2) TDI_STATE I thought at least RMM did this with an RSI?

CCA uses RHI. The RHI reaches the VMM, which issues the ioctl to complete the request.

Quoted 6 lines

3) ENABLE/DISABLE DMA, see #1, must not be done by hypervisor See my reply to Alexey:

   https://lore.kernel.org/linux-iommu/[email protected]/

4) READ/REGEN/INFO, what are the objects? What format are they in? How do you select them?

These objects are cached by the hypervisor. For CCA, they include the certificate, measurement, interface report, and VCA. They are read in response to guest requests. The requests reach the VMM through an RHI, and the VMM issues the ioctl to read the requested object. REGEN refreshes the objects/evidence at the guest's request.

Quoted 3 lines

Does this overlap with attestation subsystem? Should attestation subsystem be used to access the SPDM related data?

I had hoped we could use it, but I could not find an easy way to integrate the two. We need a simple way to read these evidence objects into userspace. I am not yet sure whether evidence-get can be used for that, since it does more than simply read the objects; it also updates evidence.generation, among other things.

Quoted 7 lines

I think this is probably a fine generic API, it should probably not be "TSM request" so that the other viommu people can reuse the transport.

I would probably try to stick to exactly like the other APIs do with a single enum. Two enums for op/arch is saying already that the ops are not well defined.

Quoted 3 lines

So maybe strip this patch out to have no tsm stuff, define no ops and just give a command transport into the viommu driver?

Are you suggesting one ioctl per each guest request ops?

From: Jason Gunthorpe <[email protected]>
Sent: Thursday, October 29, 2026 9:01 PM

Quoted 5 lines · Aneesh Kumar K.V

Quoted 3 lines · Jason Gunthorpe

Quoted 3 lines

[ ... 300 lines skipped ... ]
+/**
+ * enum iommu_vdevice_tsm_guest_req_op - Guest TSM request

operations

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ * @TSM_REQ_VALIDATE_MMIO: Validate and map a guest MMIO range

for the trusted

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+ * device.
+ * @TSM_REQ_SET_TDI_STATE: Transition the trusted device to the

unlocked,

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

+ * locked, or running state.
+ * @TSM_REQ_SEV_ENABLE_DMA: Enable DMA for an SEV device.
+ * @TSM_REQ_SEV_DISABLE_DMA: Disable DMA for an SEV device.
+ * @TSM_REQ_READ_OBJECT: Read bytes from a device object, such as a

certificate,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+ * measurement, or interface report.
+ * @TSM_REQ_REGEN_OBJECT: Regenerate a device object, such as a

measurement or

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

+ * interface report.
+ * @TSM_REQ_OBJECT_INFO: Query the size of a device object.
+ * @TSM_REQ_MAX: One past the last request operation; not a

command.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

+ *
+ * The request payload and response format depend on the TVM

architecture.

Quoted 22 lines

Quoted 14 lines

include/uapi/linux/iommufd.h struct iommu_hw_queue_alloc { · line 1433

Quoted 11 lines

+ */

include/uapi/linux/iommufd.h +10 -0

+enum iommu_vdevice_tsm_guest_req_op {
+	TSM_REQ_VALIDATE_MMIO = 1,
+	TSM_REQ_SET_TDI_STATE,
+	TSM_REQ_SEV_ENABLE_DMA,
+	TSM_REQ_SEV_DISABLE_DMA,
+	TSM_REQ_READ_OBJECT,
+	TSM_REQ_REGEN_OBJECT,
+	TSM_REQ_OBJECT_INFO,
+	TSM_REQ_MAX,
+};

This is still no where close to being well defined enough.

Quoted 7 lines

Everything here must tie back to defined spec references, listed in the comments.

I cannot guess what data is passed over any of these APIs, how things like "REGEN_OBJECT" select which object to regen, or what the format of those objects even is. The formats of req/resp are never specified.

Quoted 4 lines

The request and response payload formats are specific to each CoCo backend. Since iommufd treats the payloads as opaque and does not validate their contents, what would documenting those formats here achieve?

Well if thats the case then no, this is entirely inappropriate.

iommufd requires every single struct to be documented and described. We do not do undocumented opaque structs into drivers.

So figure out exactly what needs to flow here, give it driver specific names for driver specific content and document everything.

This should be done patch by patch as the drivers implement their stuff.

Quoted 7 lines

Quoted 4 lines

My guesses with no information
1) VALIDATE_MMIO why would the hypervisor ever need this? Guest validates MMIO through secure communication to the secure world.

With VFIO PCI BARs backed by guest_memfd, the VMM can validate that the IPA and PA values provided by the guest belong to the VFIO device.

Why? Who cares? It isn't a trusted validation, and there is a RSI that duplicates this?

Quoted 6 lines

We previously also used this operation to ask the CoCo TSM backend to map the device memory (the BAR regions). I have removed this operation from the latest series because that responsibility is now split between the VMM and guest_memfd. TDX and SNP still need to be updated accordingly.

Right, it should happen automatically as part of lock.

Quoted 4 lines

Quoted 1 line

2) TDI_STATE I thought at least RMM did this with an RSI?

CCA uses RHI. The RHI reaches the VMM, which issues the ioctl to complete the request.

And then the seq numbers check that this went through OK?

Quoted 4 lines

Quoted 4 lines

3) ENABLE/DISABLE DMA, see #1, must not be done by hypervisor See my reply to Alexey:
https://lore.kernel.org/linux-

iommu/[email protected]/

Quoted 15 lines

Quoted 3 lines

4) READ/REGEN/INFO, what are the objects? What format are they in? How do you select them?

These objects are cached by the hypervisor. For CCA, they include the certificate, measurement, interface report, and VCA. They are read in response to guest requests. The requests reach the VMM through an RHI, and the VMM issues the ioctl to read the requested object. REGEN refreshes the objects/evidence at the guest's request.

Quoted 3 lines

Does this overlap with attestation subsystem? Should attestation subsystem be used to access the SPDM related data?

Quoted 5 lines

I had hoped we could use it, but I could not find an easy way to integrate the two. We need a simple way to read these evidence objects into userspace. I am not yet sure whether evidence-get can be used for that, since it does more than simply read the objects; it also updates evidence.generation, among other things.

It should not do that, the generation has to follow the locked state, not the evidence collection.. See my remark to Dave

I think the bigger issue was it had to use the VDEV flavour not the PDEV flavor of RMM commands, and it isn't obvious how we switch from PDEV to VDEV mode on the hypervisor..

Then if your suggestion of using the attestation subsystem here can work, those OBJECT related operations are generalized too.

In the end the set of vendor specific definition here might be small.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

My guesses with no information
1) VALIDATE_MMIO why would the hypervisor ever need this? Guest validates MMIO through secure communication to the secure world.

Quoted 5 lines

Quoted 2 lines

With VFIO PCI BARs backed by guest_memfd, the VMM can validate that the IPA and PA values provided by the guest belong to the VFIO device.

Why? Who cares? It isn't a trusted validation, and there is a RSI that duplicates this?

TDX doesn't require this. It's a guest call directly to the secure world.

Quoted 1 line

And then the seq numbers check that this went through OK?

which seq numbers in your mind?

Quoted 3 lines

I think the bigger issue was it had to use the VDEV flavour not the PDEV flavor of RMM commands, and it isn't obvious how we switch from PDEV to VDEV mode on the hypervisor..

Can you elaborate this part in a general way? Want to better understand it in the TDX context...

Guest TSM requests can now be dispatched through the IOMMUFD vdevice operation. Remove the PCI-device-based guest request entry point, its scope enum and the corresponding PCI/TSM driver callback.

An IOMMUFD vdevice must use the same TEE Security Manager as its vIOMMU. Pinning the vIOMMU backend keeps its tsm_dev registered, but does not preserve the connection between the PF0 PCI device and that TSM.

Add a per-vdevice PCI TSM context. Verify that the PCI device and the vIOMMU use the same tsm_dev, and account active contexts on PF0. Reject disconnect with -EBUSY until all dependent vdevices release their contexts.

With vdevice binding handled by the backend, PCI/TSM no longer has generic bind state to report. Remove the legacy bind and unbind interface. The sysfs "bound" attribute reported the state maintained by that interface. Remove the attribute and its ABI documentation as well.

Cc [email protected]

Cc [email protected]

Signed-off-by Aneesh Kumar K.V (Arm) <[email protected]>

---
 Documentation/ABI/testing/sysfs-bus-pci |  17 +-
 drivers/pci/tsm.c                       | 230 ++++++++++--------------
 include/linux/pci-tsm.h                 |  71 +++++---
 3 files changed, 137 insertions(+), 181 deletions(-)

Documentation/ABI/testing/sysfs-bus-pci +0 -0

index 55ea1db749a180..4c64b30544cdca 100644