iommufd: Fix vDEVICE allocation lifecycle bugs

Sashiko flagged a few bugs in how IOMMU_VDEVICE_ALLOC creates and validates a vDEVICE on a vIOMMU:

- the core publishes a vDEVICE into the vIOMMU xarray before the driver's vdevice_init() runs, so a concurrent invalidation can reach one it has not yet accepted;
- the undersized-vdevice_size guard returns holding the igroup mutex, deadlocking later vDEVICE operations on that group;
- the Arm SMMUv3 vIOMMU accepts a device without exactly one Stream ID:
an out-of-bounds streams[] read for none, stale ATC/IOTLB for several.

Fix each of them properly.

This is on Github:
https://github.com/nicolinc/iommufd/commits/fix_vdevice_sashiko-v1

Nicolin Chen (3):
iommufd/viommu: Release the igroup lock on the vdevice_size error path iommufd/viommu: Publish a vDEVICE only after vdevice_init() succeeds iommu/arm-smmu-v3-iommufd: Require exactly one Stream ID for a vDEVICE

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 15 +++++++++++++++
 drivers/iommu/iommufd/viommu.c                  | 17 +++++++++++------
 2 files changed, 26 insertions(+), 6 deletions(-)

base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482

2.43.0

iommufd_vdevice_alloc_ioctl() takes idev->igroup->lock, then validates the driver's vdevice_size against the core structure size with a WARN_ON_ONCE. On failure that guard jumps to out_put_idev, below out_unlock_igroup, so it skips the mutex_unlock(), leaving the igroup lock held and deadlocking the next vDEVICE operation on that group.

Jump to out_unlock_igroup instead.

---
 drivers/iommu/iommufd/viommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 4081deda9b33d..0c12c7e352a14 100644
@@ -189,7 +189,7 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 		if (WARN_ON_ONCE(viommu->ops->vdevice_size < vdev_size ||
 				 !viommu->ops->vdevice_init)) {
 			rc = -EOPNOTSUPP;
-			goto out_put_idev;
+			goto out_unlock_igroup;
 		}
 		vdev_size = viommu->ops->vdevice_size;
 	}

2.43.0

iommufd_vdevice_alloc_ioctl() adds the vDEVICE to the viommu->vdevs xarray with xa_cmpxchg() before the driver's vdevice_init() op runs. That op is where a driver validates the device and may reject it, but the xarray entry is already live by then: a concurrent IOMMU_HWPT_INVALIDATE can look it up with iommufd_viommu_find_dev() and run the driver invalidation path against a device that vdevice_init() would have refused.

Reserve the index with xa_insert(): it stores a zero entry that reads back as NULL, and returns -EBUSY on a duplicate virt_id. Run vdevice_init() and store the vDEVICE pointer only once it succeeds. A failed vdevice_init() releases the reservation, so lookups observe the vDEVICE only after it is fully initialized and accepted.

---
 drivers/iommu/iommufd/viommu.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

index 0c12c7e352a14..5b40e924f0782 100644
@@ -143,7 +143,7 @@ void iommufd_vdevice_destroy(struct iommufd_object *obj)
 int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 {
 	struct iommu_vdevice_alloc *cmd = ucmd->cmd;
-	struct iommufd_vdevice *vdev, *curr;
+	struct iommufd_vdevice *vdev;
 	size_t vdev_size = sizeof(*vdev);
 	struct iommufd_viommu *viommu;
 	struct iommufd_device *idev;

@@ -218,18 +218,21 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	 */
 	idev->vdev = vdev;
 
-	curr = xa_cmpxchg(&viommu->vdevs, virt_id, NULL, vdev, GFP_KERNEL);
-	if (curr) {
-		rc = xa_err(curr) ?: -EEXIST;
+	rc = xa_insert(&viommu->vdevs, virt_id, NULL, GFP_KERNEL);
+	if (rc) {
+		if (rc == -EBUSY)
+			rc = -EEXIST;
 		goto out_abort;
 	}
 
 	if (viommu->ops && viommu->ops->vdevice_init) {
 		rc = viommu->ops->vdevice_init(vdev);
 		if (rc)
-			goto out_abort;
+			goto out_release;
 	}
 
+	xa_store(&viommu->vdevs, virt_id, vdev, GFP_KERNEL);
+
 	cmd->out_vdevice_id = vdev->obj.id;
 	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
 	if (rc)

@@ -237,6 +240,8 @@ int iommufd_vdevice_alloc_ioctl(struct iommufd_ucmd *ucmd)
 	iommufd_object_finalize(ucmd->ictx, &vdev->obj);
 	goto out_unlock_igroup;
 
+out_release:
+	xa_release(&viommu->vdevs, virt_id);
 out_abort:
 	iommufd_object_abort_and_destroy(ucmd->ictx, &vdev->obj);
 out_unlock_igroup:

2.43.0

cc stable

		if (rc) {
			xa_release(&viommu->vdevs, virt_id);
			goto out_abort;
		}

arm_vsmmu_vsid_to_sid() maps a guest's vSID to a single physical Stream ID taken from master->streams[0], assuming a device has exactly one stream. A device with several streams gets only its first one mapped, so a guest vSID invalidation cannot reach the others' ATC and IOTLB entries; a device with none makes master->streams a ZERO_SIZE_PTR, read out of bounds.

Add an arm_vsmmu_vdevice_init() op to reject the vDEVICE with -EINVAL when master->num_streams is not one, rather than mapping it silently.

---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c   | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

index 1e9f7d2de3441..2ba08df75af8b 100644
@@ -297,6 +297,20 @@ static int arm_vsmmu_vsid_to_sid(struct arm_vsmmu *vsmmu, u32 vsid, u32 *sid)
 	return ret;
 }
 
+static int arm_vsmmu_vdevice_init(struct iommufd_vdevice *vdev)
+{
+	struct device *dev = iommufd_vdevice_to_device(vdev);
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+
+	/*
+	 * arm_vsmmu_vsid_to_sid() maps a vSID to master->streams[0] alone, so
+	 * more streams would leave the rest stale and none reads out of bounds.
+	 */
+	if (master->num_streams != 1)
+		return -EINVAL;
+	return 0;
+}
+
 /* This is basically iommu_viommu_arm_smmuv3_invalidate in u64 for conversion */
 struct arm_vsmmu_invalidation_cmd {
 	union {

@@ -403,6 +417,7 @@ int arm_vsmmu_cache_invalidate(struct iommufd_viommu *viommu,
 static const struct iommufd_viommu_ops arm_vsmmu_ops = {
 	.alloc_domain_nested = arm_vsmmu_alloc_domain_nested,
 	.cache_invalidate = arm_vsmmu_cache_invalidate,
+	.vdevice_init = arm_vsmmu_vdevice_init,
 };
 
 size_t arm_smmu_get_viommu_size(struct device *dev,

2.43.0

OK. I will change it and other places too.

Thanks
Nicolin