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
