Re: [PATCH v2 12/14] iommu: Consolidate the default_domain setup to one function

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2023/3/30 7:40, Jason Gunthorpe wrote:
> +/**
> + * iommu_setup_default_domain - Set the default_domain for the group
> + * @group: Group to change
> + * @target_type: Domain type to set as the default_domain
> + *
> + * Allocate a default domain and set it as the current domain on the group. If
> + * the group already has a default domain it will be changed to the target_type.
> + * When target_type is 0 the default domain is selected based on driver and
> + * system preferences.
> + */
> +static int iommu_setup_default_domain(struct iommu_group *group,
> +				      int target_type)
> +{
> +	struct group_device *gdev;
> +	struct iommu_domain *dom;
> +	struct bus_type *bus =
> +		list_first_entry(&group->devices, struct group_device, list)
> +			->dev->bus;
> +	int ret;
> +
> +	lockdep_assert_held(&group->mutex);
> +
> +	target_type = iommu_get_default_domain_type(group, target_type);
> +	if (target_type < 0)
> +		return -EINVAL;
> +
> +	if (group->default_domain && group->default_domain->type == target_type)
> +		return 0;
> +
> +	dom = __iommu_domain_alloc(bus, target_type);
> +	if (!dom && target_type != IOMMU_DOMAIN_DMA) {
> +		dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
> +		if (dom)
> +			pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
> +				target_type, group->name);
> +	}

The background of the code above is that some ARM IOMMU drivers only
support DMA mapping domain and do not support identity domain.
Therefore, during boot, if the allocation of identity domain fails, a
DMA mapping domain is used instead.

However, this does not apply to use cases that change the default domain
through sysfs. In such cases, it seems that we should directly return
failure (-ENODEV) and tell the user that the iommu driver does not
support identity domain.

> +
> +	/*
> +	 * There are still some drivers which don't support default domains, so
> +	 * we ignore the failure and leave group->default_domain NULL.
> +	 *
> +	 * We assume that the iommu driver starts up the device in
> +	 * 'set_platform_dma_ops' mode if it does not support default domains.
> +	 */
> +	if (!dom) {
> +		ret = 0;
> +		goto out_set;
> +	}

Should we call set_platform_dma_ops here? The existing default domain
(if exists) will be freed below. But the iommu driver doesn't know about
this. It probably will create a UAF case?

> +
> +	ret = __iommu_group_set_domain_internal(group, dom,
> +						IOMMU_SET_DOMAIN_WITH_DEFERRED);
> +	if (ret) {
> +		/*
> +		 * An attach_dev failure may result in some devices being left
> +		 * attached to dom. This is not cleaned up until release_device
> +		 * is called. Thus we can't always free dom on failure, we have
> +		 * no choice but to stick the broken domain into
> +		 * group->default_domain to defer the free and try to continue.
> +		 */
> +		if (list_count_nodes(&group->devices) > 1)
> +			goto out_set;
> +
> +		iommu_domain_free(dom);
> +		dom = NULL;
> +		goto out_set;
> +	}
> +
> +	/* The domain must be attached before we can establish any mappings */
> +	for_each_group_device(group, gdev)
> +		iommu_create_device_direct_mappings(dom, gdev->dev);

It's better to move creating direct mappings before setting the domain
to the group devices.

The VT-d platforms allow the firmware to access the memory regions
defined in RMRR ACPI table. If we set an empty domain to the device
while the firmware DMA accesses the RMRR memory, it might result in
spurious DMA faults.

> +
> +out_set:
> +	if (group->default_domain)
> +		iommu_domain_free(group->default_domain);
> +	group->default_domain = dom;
> +	return ret;
> +}

Best regards,
baolu