Re: [PATCH 1/4] accel/rocket: Fix the IOMMU domain leak in rocket_ioctl_create_bo

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The return value of `iommu_map_sgtable` (`ssize_t`) is stored in an `int`, causing an overflow to a negative value for BOs of 2GB or larger, leading to a massive IOMMU mapping leak.
- [High] In `rocket_ioctl_prep_bo`, `dma_sync_sgtable_for_cpu` is called unconditionally even if `dma_resv_wait_timeout` fails with a timeout or error.
- [High] The per-file `rocket_priv` context is freed on file close, but GEM objects can outlive the file descriptor, causing a Use-After-Free when they are eventually destroyed.
--

--- Patch [1]: [PATCH 1/4] accel/rocket: Fix the IOMMU domain leak in rocket_ioctl_create_bo ---
commit ea4421559353add8c0bf8f5cce3e937cd76171d8
Author: Chaoyi Chen <[email protected]>

accel/rocket: Fix the IOMMU domain leak in rocket_ioctl_create_bo
    
When rocket_ioctl_create_bo fails, rocket_iommu_domain_put should be
called to avoid an IOMMU domain leak.

> diff --git a/drivers/accel/rocket/rocket_gem.c b/drivers/accel/rocket/rocket_gem.c
> index a5fffa51ff355..438694ba567b9 100644
> --- a/drivers/accel/rocket/rocket_gem.c
> +++ b/drivers/accel/rocket/rocket_gem.c
> @@ -127,6 +127,8 @@ int rocket_ioctl_create_bo(struct drm_device *dev, void *data, struct drm_file *
>  	mutex_unlock(&rocket_priv->mm_lock);
>  
>  err:
> +	rocket_iommu_domain_put(rkt_obj->domain);
> +	rkt_obj->domain = NULL;
>  	drm_gem_shmem_object_free(gem_obj);
>  
>  	return ret;

[Severity: Critical]
This is a pre-existing issue, but does rocket_ioctl_create_bo() in 
drivers/accel/rocket/rocket_gem.c truncate the return value of
iommu_map_sgtable() to a signed integer?

	ret = iommu_map_sgtable(rocket_priv->domain->domain,
				rkt_obj->mm.start,
				shmem_obj->sgt,
				IOMMU_READ | IOMMU_WRITE);
	if (ret < 0 || ret < args->size) {

If a user requests a buffer size of 2 GiB, iommu_map_sgtable() successfully
maps it and returns 2147483648 (0x80000000) as ssize_t. Is this truncated
into the local int ret, becoming -2147483648, which triggers the ret < 0
check? This would wrongly treat success as failure, bypass the IOMMU unmap
logic, and leak the IOMMU mappings for the freed backing pages.

[Severity: High]
This isn't a bug introduced by this patch, but can the per-file
rocket_priv context be freed while exported GEM objects still exist?

When a userspace client creates a GEM object, exports it, and then closes
its DRM file descriptor, rocket_postclose() in
drivers/accel/rocket/rocket_drv.c destructs rocket_priv:

	mutex_destroy(&rocket_priv->mm_lock);
	drm_mm_takedown(&rocket_priv->mm);
	rocket_iommu_domain_put(rocket_priv->domain);
	kfree(rocket_priv);

Later, when the final reference to the exported GEM object is dropped, would
rocket_gem_bo_free() in drivers/accel/rocket/rocket_gem.c dereference the
freed rocket_priv?

	mutex_lock(&rocket_priv->mm_lock);
	drm_mm_remove_node(&bo->mm);
	mutex_unlock(&rocket_priv->mm_lock);

This could lead to a use-after-free since the GEM object outlives the file
descriptor.

[Severity: High]
This is a pre-existing issue, but does rocket_ioctl_prep_bo() in
drivers/accel/rocket/rocket_gem.c unconditionally sync the buffer even if
the wait fails?

	ret = dma_resv_wait_timeout(gem_obj->resv, DMA_RESV_USAGE_WRITE, true, timeout);
	if (!ret)
		ret = timeout ? -ETIMEDOUT : -EBUSY;
	else if (ret > 0)
		ret = 0;

	shmem_obj = &to_rocket_bo(gem_obj)->base;

	dma_sync_sgtable_for_cpu(dev->dev, shmem_obj->sgt, DMA_BIDIRECTIONAL);

If dma_resv_wait_timeout() fails due to a timeout or interrupted wait
and returns a negative error code, the code continues executing without
an early return. Could this lead to data corruption since
dma_sync_sgtable_for_cpu() is invoked while the GPU may still be actively
writing to the buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1