Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Integer overflow in `iommu_map_sgtable()` return value check causes >= 2GB BO creations to fail and leak IOMMU mappings. - [High] The `rocket_gem_bo_free` function accesses `bo->driver_priv`, leading to a Use-After-Free if the GEM object outlives the `drm_file`. -- --- Patch [1]: [PATCH] accel/rocket: put IOMMU domain on BO creation failure --- commit cee2a8e5b9e315a984b6639a6737db57bbbb72ca Author: Guangshuo Li <[email protected]> accel/rocket: put IOMMU domain on BO creation failure The BO creation error paths free the object with drm_gem_shmem_object_free(), which bypasses the driver's GEM .free callback and releases the shmem object without dropping the IOMMU domain reference. This commit drops the domain reference explicitly on the BO creation error path. > 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: [Severity: Critical] This is a pre-existing issue, but does the return value check for iommu_map_sgtable() earlier in this function have an integer overflow? Because iommu_map_sgtable() returns a ssize_t, and rocket_ioctl_create_bo() stores this in a 32-bit signed int "ret", a mapping of 2GB or more would result in a negative value. When this happens, the check evaluates to true: ret = iommu_map_sgtable(rocket_priv->domain->domain, ...); if (ret < 0 || ret < args->size) { drm_err(dev, "failed to map buffer..."); ret = -ENOMEM; goto err_remove_node; } This incorrectly triggers the error path and bypasses the err_unmap label, leaving the IOMMU mapping active while freeing the IOVA and physical pages. Could this allow arbitrary physical memory access if the device is instructed to perform DMA to the freed IOVA? [Severity: High] This is a pre-existing issue, but is there a use-after-free risk in rocket_gem_bo_free()? If a user exports the GEM object to a DMA-BUF using generic PRIME ioctls and then closes the DRM file descriptor, the rocket_file_priv is freed. When the DMA-BUF is later closed, the GEM object's refcount drops to zero, invoking rocket_gem_bo_free(). This attempts to access the freed memory: rocket_gem_bo_free() { ... struct rocket_file_priv *rocket_priv = bo->driver_priv; ... } Could this lead to accessing a freed lock when attempting to lock rocket_priv->mm_lock? > + rocket_iommu_domain_put(rkt_obj->domain); > + rkt_obj->domain = NULL; > drm_gem_shmem_object_free(gem_obj); > > return ret; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
