Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
Pre-existing issues: - [Critical] NULL pointer dereference when opening the DRM device before core initialization completes. - [Critical] Use-After-Free due to deferred DRM device unregistration during individual core teardown. - [High] Massive memory and resource leak due to devres allocations on a permanent platform device. - [High] Out-of-bounds access and data corruption due to missing array compaction on core removal. - [High] Race condition and data races on global state during concurrent core probes. - [High] Memory leak of IOMMU domain reference in `rocket_ioctl_create_bo` error path. - [High] Use-After-Free and NULL pointer dereference in concurrent timeout handler during teardown. - [High] Missing clock IDs in `clk_bulk_data` causes failure to properly request and enable hardware clocks. -- --- Patch [1]: [PATCH] accel/rocket: clear rdev on device init failure --- commit 17e3337636f75d45b7220a5f2dd42ebed6a265c7 Author: Guangshuo Li <[email protected]> accel/rocket: clear rdev on device init failure This patch updates rocket_probe() to clear the file-scoped rdev pointer and return the saved error code if rocket_device_init() fails. This prevents a later probe from seeing a non-NULL rdev and dereferencing an error pointer. > diff --git a/drivers/accel/rocket/rocket_drv.c > b/drivers/accel/rocket/rocket_drv.c > index 8bbbce594883e..67e7f543fe4c7 100644 > --- a/drivers/accel/rocket/rocket_drv.c > +++ b/drivers/accel/rocket/rocket_drv.c > @@ -165,8 +165,11 @@ static int rocket_probe(struct platform_device *pdev) > /* First core probing, initialize DRM device. */ > rdev = rocket_device_init(drm_dev, &rocket_drm_driver); > if (IS_ERR(rdev)) { > + int err = PTR_ERR(rdev); > + > dev_err(&pdev->dev, "failed to initialize rocket > device\n"); > - return PTR_ERR(rdev); > + rdev = NULL; [Severity: High] This is a pre-existing issue, but does this error path permanently leak memory? In drivers/accel/rocket/rocket_device.c:rocket_device_init(), allocations are made using devres bound to the persistent drm_dev: rdev = devm_drm_dev_alloc(dev, rocket_drm_driver, struct rocket_device, ddev); Since drm_dev is a static platform device that persists until module unload, these devres allocations are never freed. If rocket_probe() fails here, abandoning the devm-managed allocations without freeing them, will successive probe failures or unbind/bind cycles continuously stack unbounded memory leaks? > + return err; > } > } > [Severity: Critical] This isn't a bug introduced by this patch, but could this cause a NULL pointer dereference when opening the DRM device? In drivers/accel/rocket/rocket_device.c:rocket_device_init(), the device is registered and exposed to userspace prematurely: err = drm_dev_register(ddev, 0); If an unprivileged user opens the DRM device right after registration, before rocket_probe() finishes initializing the hardware cores, rocket_open() is called: rocket_priv->domain = rocket_iommu_domain_create(rdev->cores[0].dev); Since cores[0].dev is still NULL, this passes a NULL device pointer to iommu_paging_domain_alloc(), which would cause a kernel panic when it dereferences dev->iommu. [Severity: Critical] This is a pre-existing issue, but does deferred DRM device unregistration lead to a use-after-free during individual core teardown? When an individual core is unbound, drivers/accel/rocket/rocket_drv.c:rocket_remove() destroys that core's drm_gpu_scheduler: rocket_core_fini(&rdev->cores[core]); However, drm_dev_unregister() is deferred until the very last core is removed. Open file descriptors can still access the device, and drivers/accel/rocket/rocket_job.c:rocket_job_open() will initialize a drm_sched_entity pointing to the destroyed scheduler: scheds[core] = &rdev->cores[core].sched; Could subsequent user ioctls to submit jobs push them to a destroyed scheduler, causing a use-after-free crash? [Severity: High] This isn't a bug introduced by this patch, but is there an out-of-bounds access due to missing array compaction on core removal? Removing a core decrements the count without compacting the array: rdev->num_cores--; As a result, find_core_for_dev() iterations will truncate and fail to locate active cores at higher indices. Furthermore, if a new core is probed, drivers/accel/rocket/rocket_drv.c:rocket_probe() uses the artificially lowered num_cores as the index: rdev->cores[core].rdev = rdev; Would this overwrite an existing active core slot, destroying its state and corrupting the array mapping? [Severity: High] This is a pre-existing issue, but is there a data race on global state during concurrent core probes? The global rdev pointer and rdev->num_cores counter are accessed and modified without synchronization. Concurrent probes for different device tree nodes could race here in drivers/accel/rocket/rocket_drv.c:rocket_probe(): if (rdev == NULL) { and later increment without synchronization: rdev->num_cores++; Could multiple threads simultaneously pass the NULL check, leading to duplicate memory allocations, corrupting the rdev->cores array, and losing core registrations? [Severity: High] This isn't a bug introduced by this patch, but does the error path in rocket_ioctl_create_bo leak the IOMMU domain reference? If BO creation fails, the error path directly calls: drm_gem_shmem_object_free(gem_obj); This frees the GEM object but completely bypasses the driver's custom rocket_gem_bo_free() callback. As a result, the IOMMU domain reference acquired earlier is never released: rkt_obj->domain = rocket_iommu_domain_get(rocket_priv); Could a malicious userspace program exploit this to cause an out of memory condition by repeatedly triggering mapping failures? [Severity: High] This is a pre-existing issue, but could this lead to a use-after-free and NULL pointer dereference in a concurrent timeout handler? During unbind, drivers/accel/rocket/rocket_core.c:rocket_core_fini() drops the IOMMU group reference and clears the pointer before tearing down the scheduler: core->iommu_group = NULL; If a job timeout triggers concurrently, drivers/accel/rocket/rocket_job.c:rocket_reset() is scheduled and calls: iommu_detach_group(NULL, core->iommu_group); Does this dereference the now-NULL core->iommu_group pointer inside iommu_detach_group? [Severity: High] This isn't a bug introduced by this patch, but does the lack of clock IDs cause a failure to properly request and enable hardware clocks? In drivers/accel/rocket/rocket_core.c:rocket_core_init(), clocks are fetched using an uninitialized array where the .id fields are NULL: err = devm_clk_bulk_get(dev, ARRAY_SIZE(core->clks), core->clks); When .id is NULL, the clock subsystem falls back to fetching the very first clock listed in the device tree for the device. Does this result in the driver obtaining four references to the exact same clock instead of the four distinct required clocks, causing hardware functional failure during power management? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
