Re: [PATCH v3 07/16] accel/thames: Add IOCTLs for BO creation and mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] accel/thames: use-after-free of bo in thames_ioctl_bo_create()
- [High] accel/thames: missing TLB flush when thames_bo_bind() replaces L3 table
- [Low] accel/thames: Kconfig selects DRM_GEM_SHMEM_HELPER without MMU dependency
- [Low] accel/thames: missing MMU, IOMMU_SUPPORT, and 64BIT dependencies in Kconfig
--

commit e7110c2d02687ac644a69317b2f94c6b4caee594
Author: Tomeu Vizoso <tomeu.vizoso@gmail.com>

accel/thames: Add IOCTLs for BO creation and mapping

This commit adds the DRM_IOCTL_THAMES_BO_CREATE,
DRM_IOCTL_THAMES_BO_MMAP_OFFSET, and DRM_IOCTL_THAMES_BO_BIND ioctls to
create SHMEM or contiguous DMA buffer objects and map or unmap them in
the DSP page tables of each core.

> diff --git a/drivers/accel/thames/Kconfig b/drivers/accel/thames/Kconfig
> index 9362b92740751..47a2986fcabdc 100644
> --- a/drivers/accel/thames/Kconfig
> +++ b/drivers/accel/thames/Kconfig
> @@ -6,6 +6,8 @@ config DRM_ACCEL_THAMES
>  	depends on TI_K3_DSP_REMOTEPROC
>  	depends on REMOTEPROC
>  	depends on MAILBOX
> +	select DRM_GEM_DMA_HELPER
> +	select DRM_GEM_SHMEM_HELPER
>  	select IOMMU_IO_PGTABLE_LPAE

[Severity: Low]
Can selecting DRM_GEM_SHMEM_HELPER and IOMMU_IO_PGTABLE_LPAE without
inheriting their dependencies cause Kconfig unmet dependency warnings,
build failures, or 32-bit address truncation when COMPILE_TEST is enabled?

In drivers/remoteproc/Kconfig, TI_K3_DSP_REMOTEPROC has:

	config TI_K3_DSP_REMOTEPROC
		tristate "TI K3 DSP remoteproc support"
		depends on ARCH_K3 || COMPILE_TEST

However, DRM_GEM_SHMEM_HELPER in drivers/gpu/drm/Kconfig depends on MMU:

	config DRM_GEM_SHMEM_HELPER
		tristate
		depends on DRM && MMU

And IOMMU_IO_PGTABLE_LPAE in drivers/iommu/Kconfig is guarded by
if IOMMU_SUPPORT (which depends on MMU) and depends on !GENERIC_ATOMIC64,
with drivers/iommu/io-pgtable-arm.c also calling iommu_alloc_pages_node_sz()
and iommu_free_pages() from CONFIG_IOMMU_SUPPORT:

	if IOMMU_SUPPORT
	...
	config IOMMU_IO_PGTABLE_LPAE
		bool "ARMv7/v8 Long Descriptor Format"
		select IOMMU_IO_PGTABLE
		depends on ARM || ARM64 || COMPILE_TEST
		depends on !GENERIC_ATOMIC64	# for cmpxchg64()

In addition, thames_bo_bind() requires dsp_va >= THAMES_BIND_VA_START
(SZ_4G) and passes dsp_va through thames_context_map() ->
thames_pgtable_map() -> io_pgtable_ops->map_pages() and ->unmap_pages(),
where the iova parameter is unsigned long and would truncate 64-bit DSP
virtual addresses on 32-bit COMPILE_TEST builds.

Should DRM_ACCEL_THAMES depend on MMU, IOMMU_SUPPORT (or select it as
DRM_PANFROST and DRM_PANTHOR do), !GENERIC_ATOMIC64, and 64BIT?

[ ... ]

> diff --git a/drivers/accel/thames/thames_gem.c b/drivers/accel/thames/thames_gem.c
> index b98e0fe203b08..c9fd08c7aaf4d 100644
> --- a/drivers/accel/thames/thames_gem.c
> +++ b/drivers/accel/thames/thames_gem.c

[ ... ]

> +static int thames_bo_bind(struct thames_file_priv *priv, struct thames_gem_object *bo,
> +			  u64 dsp_va)
> +{
> +	size_t size = bo->base.base.size;
> +	int prot = thames_bo_flags_to_prot(bo->flags);
> +	struct thames_binding *binding;
> +	int ret;
> +
> +	if (!IS_ALIGNED(dsp_va, PAGE_SIZE))
> +		return -EINVAL;
> +
> +	if (dsp_va < THAMES_BIND_VA_START || dsp_va > THAMES_BIND_VA_END ||
> +	    size > THAMES_BIND_VA_END - dsp_va)
> +		return -EINVAL;
> +
> +	binding = kzalloc_obj(*binding);
> +	if (!binding)
> +		return -ENOMEM;
> +
> +	binding->bo = bo;
> +	binding->priv = priv;
> +	binding->mm.start = dsp_va;
> +	binding->mm.size = size;
> +
> +	ret = mutex_lock_interruptible(&priv->tdev->bindings_lock);
> +	if (ret)
> +		goto err_free;
> +
> +	if (thames_bo_find_binding(bo, priv)) {
> +		ret = -EBUSY;
> +		goto err_unlock;
> +	}
> +
> +	scoped_guard(mutex, &priv->mm_lock)
> +		ret = drm_mm_reserve_node(&priv->mm, &binding->mm);
> +	if (ret)
> +		goto err_unlock;
> +
> +	if (bo->flags & DRM_THAMES_BO_CONTIGUOUS)
> +		ret = thames_context_map(priv, dsp_va, prot, bo->base.dma_addr, size);
> +	else
> +		ret = thames_context_map_sgt(priv, dsp_va, prot, bo->shmem.sgt);
> +	if (ret) {
> +		scoped_guard(mutex, &priv->mm_lock)
> +			drm_mm_remove_node(&binding->mm);
> +		goto err_unlock;
> +	}
> +
> +	list_add_tail(&binding->bo_link, &bo->bindings);
> +	list_add_tail(&binding->context_link, &priv->bindings);
> +	mutex_unlock(&priv->tdev->bindings_lock);

[Severity: High]
Can thames_bo_bind() leave stale walk-cache entries on the DSP cores when a
2 MiB block mapping replaces an existing Level 3 page table?

In thames_context_pgtable_init(), the ARM_64_LPAE_S1 page tables are
allocated with cfg.tlb left as NULL:

	struct io_pgtable_cfg cfg = {
		.pgsize_bitmap = SZ_4K | SZ_2M,
		.ias = 48,
		.oas = 40,
		.coherent_walk = false,
		.iommu_dev = tdev->ddev.dev,
	};

Because cfg.tlb is NULL, io_pgtable_tlb_flush_walk() is a no-op and TLB
invalidation relies on thames_context_bump_generation(priv).

If a 4 KiB buffer is bound inside a 2 MiB region and later unbound via
thames_bo_unbind(), __arm_lpae_unmap() clears the Level 3 leaf PTE while
leaving the Level 2 ARM_LPAE_PTE_TYPE_TABLE descriptor in place. If a job
then runs on the DSP, the unbind's mapping_generation is consumed while the
Level 2 table descriptor remains valid and cacheable by the MMU walk cache.

When a 2 MiB buffer is subsequently bound at that 2 MiB-aligned dsp_va via
thames_bo_bind(), arm_lpae_init_pte() in drivers/iommu/io-pgtable-arm.c
unmaps and frees the old Level 3 page table page before installing the
block entry:

	} else if (iopte_type(ptep[i]) == ARM_LPAE_PTE_TYPE_TABLE) {
		/*
		 * We need to unmap and free the old table before
		 * overwriting it with a block entry.
		 */
		arm_lpae_iopte *tblp;
		size_t sz = ARM_LPAE_BLOCK_SIZE(lvl, data);

		tblp = ptep - ARM_LPAE_LVL_IDX(iova, lvl, data);
		if (__arm_lpae_unmap(data, NULL, iova + i * sz, sz, 1,
				     lvl, tblp) != sz) {
			WARN_ON(1);
			return -EINVAL;
		}
	}

Since neither cfg.tlb->tlb_flush_walk nor thames_bo_bind() calls
thames_context_bump_generation(priv), won't the DSP MMU walk cache retain
entries pointing to the freed Level 3 page table page?

[ ... ]

> +int thames_ioctl_bo_create(struct drm_device *ddev, void *data, struct drm_file *file)
> +{
> +	struct drm_thames_bo_create *args = data;
> +	struct thames_gem_object *bo;
> +	struct drm_gem_dma_object *mem;
> +	int ret;
> +
> +	if (args->handle)
> +		return -EINVAL;
> +
> +	if (!args->size)
> +		return -EINVAL;
> +
> +	if (args->flags & ~(DRM_THAMES_BO_READ_ONLY | DRM_THAMES_BO_EXECUTABLE |
> +			    DRM_THAMES_BO_CONTIGUOUS))
> +		return -EINVAL;
> +
> +	if (args->flags & DRM_THAMES_BO_CONTIGUOUS) {
> +		mem = thames_gem_create(ddev, args->size);
> +		if (IS_ERR(mem))
> +			return PTR_ERR(mem);
> +		bo = to_thames_bo(&mem->base);
> +	} else {
> +		bo = thames_gem_create_shmem(ddev, args->size);
> +		if (IS_ERR(bo))
> +			return PTR_ERR(bo);
> +	}
> +
> +	bo->flags = args->flags;
> +
> +	ret = drm_gem_handle_create(file, &bo->base.base, &args->handle);
> +	drm_gem_object_put(&bo->base.base);
> +	if (ret) {
> +		dev_err(ddev->dev, "Failed to create gem handle: %d", ret);
> +		return ret;
> +	}
> +
> +	args->size = bo->base.base.size;

[Severity: Critical]
Does reading bo->base.base.size after drm_gem_object_put(&bo->base.base)
cause a use-after-free of bo?

Once drm_gem_handle_create() publishes the handle in file->object_idr and
drm_gem_object_put() drops the local reference, a concurrent thread sharing
the same drm_file can call DRM_IOCTL_GEM_CLOSE on the new handle and free
bo via thames_gem_bo_free() or thames_gem_shmem_bo_free() before
args->size = bo->base.base.size is reached.

Should args->size = bo->base.base.size be set before calling
drm_gem_handle_create() or before dropping the local reference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-thames-v3-0-3ab40aa17157@tomeuvizoso.net?part=7