Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
New issues: - [Medium] Phantom Fix / Dead Code Pre-existing issues: - [Critical] Integer Overflow leading to Out-of-Bounds memory access in exynos_g2d_set_cmdlist_ioctl - [Critical] Out-of-Bounds Write in g2d_map_cmdlist_gem - [Critical] Use-After-Free in IRQ handler when DRM file descriptor is closed - [High] Permanent reference leak and task hang in g2d_dma_start() - [High] Data Race and Logical Bug bypassing global userptr pool limits - [High] Use-After-Free / NULL Pointer Dereference in IRQ and workqueue on removal - [High] Double Free of DMA memory in g2d_remove() - [High] Lockless access UAF to runqueue_node in IRQ handler - [Medium] Memory Leak in g2d_init_cmdlist -- --- Patch [1]: [PATCH] drm/exynos: g2d: fix autosuspend cleanup on probe failure --- commit 52d780780d9657013e887d137cd889b60f793705 Author: Guangshuo Li <[email protected]> Subject: drm/exynos: g2d: fix autosuspend cleanup on probe failure This commit adds a call to pm_runtime_dont_use_autosuspend() in the probe failure path of the g2d driver. The commit message states this is necessary to balance a corresponding call to pm_runtime_use_autosuspend() and prevent a usage_count leak. > diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c > b/drivers/gpu/drm/exynos/exynos_drm_g2d.c > index 3271fd4b17240..3222b218c8e76 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c > @@ -1448,6 +1448,7 @@ static int g2d_probe(struct platform_device *pdev) > return 0; > > err_put_clk: > + pm_runtime_dont_use_autosuspend(dev); [Severity: Medium] The commit message states that g2d_probe() calls pm_runtime_use_autosuspend(), but does the driver actually ever call pm_runtime_use_autosuspend()? Since autosuspend is never enabled for this device, it seems dev->power.use_autosuspend is already 0. Does this mean the cleanup call is unnecessary and adds dead code? > pm_runtime_disable(dev); > err_destroy_workqueue: > destroy_workqueue(g2d->g2d_workq); While reviewing this patch, I noticed several pre-existing issues in this file. > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_dma_start() { > [ ... ] > ret = pm_runtime_get_sync(g2d->dev); > if (ret < 0) > return; > [ ... ] [Severity: High] This is a pre-existing issue, but does this permanently leak the runtime PM reference if pm_runtime_get_sync() returns < 0? The pm_runtime_get_sync() function increments the usage counter even on failure. Because pm_runtime_put_noidle() is not called here, could a userspace application calling exynos_g2d_exec_ioctl() synchronously be stuck indefinitely waiting for a completion interrupt that will never fire? > drivers/gpu/drm/exynos/exynos_drm_g2d.c:exynos_g2d_set_cmdlist_ioctl() { > [ ... ] > size = cmdlist->last + req->cmd_nr * 2 + req->cmd_buf_nr * 2 + 2; > if (size > G2D_CMDLIST_DATA_NUM) { > [ ... ] [Severity: Critical] This is a pre-existing issue, but can this size calculation overflow? If an attacker provides sufficiently large values for req->cmd_nr and req->cmd_buf_nr, could the size variable (declared as signed int) become negative, bypassing the G2D_CMDLIST_DATA_NUM bounds check? This appears to allow out-of-bounds memory access later in g2d_check_reg_offset(). > drivers/gpu/drm/exynos/exynos_drm_g2d.c:exynos_g2d_set_cmdlist_ioctl() { > [ ... ] > node->buf_info.map_nr = req->cmd_buf_nr; > [ ... ] > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_map_cmdlist_gem() { > [ ... ] > for (i = 0; i < buf_info->map_nr; i++) { > buf_info->reg_types[i] = reg_type; > buf_info->handles[reg_type] = handle; > [ ... ] [Severity: Critical] This is a pre-existing issue, but does this loop write out-of-bounds if req->cmd_buf_nr is greater than MAX_REG_TYPE_NR (which is 6)? The overall command list size check permits req->cmd_buf_nr to be up to ~125. Since buf_info->reg_types is strictly dimensioned to MAX_REG_TYPE_NR, would iterations above 5 linearly overwrite adjacent fields in the struct g2d_buf_info? > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_userptr_free_all() { > [ ... ] > g2d->current_pool = 0; > [ ... ] [Severity: High] This is a pre-existing issue, but does this inadvertently reset the global device-wide current_pool counter back to zero when a single user closes its file descriptor? Since g2d->current_pool tracks memory across the entire hardware device, it seems this clears the accounting for all active clients on the system, bypassing the max_pool limit. Additionally, in g2d_userptr_get_dma_addr(), list_add_tail() modifies g2d_priv->userptr_list locklessly. Is this safe if multiple threads share the same DRM file descriptor and invoke the ioctl concurrently? > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_remove() { > [ ... ] > destroy_workqueue(g2d->g2d_workq); > [ ... ] > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_irq_handler() { > [ ... ] > if (pending & G2D_INTP_ACMD_FIN) > queue_work(g2d->g2d_workq, &g2d->runqueue_work); > [ ... ] [Severity: High] This is a pre-existing issue, but is it possible for the hardware interrupt to fire after destroy_workqueue() is called? The driver uses devm_request_irq(), meaning the IRQ is only freed by devres after g2d_remove() returns. If an interrupt fires during this window, could g2d_irq_handler() access a freed workqueue pointer? > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_subdrv_probe() { > [ ... ] > ret = drm_iommu_attach_device(drm_dev, dev); > if (ret < 0) { > dev_err(dev, "failed to enable iommu.\n"); > g2d_fini_cmdlist(g2d); > } > [ ... ] [Severity: High] This is a pre-existing issue, but if g2d_subdrv_probe() fails and calls g2d_fini_cmdlist(g2d), the DMA memory is freed, but g2d->cmdlist_pool_virt is not set to NULL. Since g2d_remove() also unconditionally calls g2d_fini_cmdlist(g2d), would this lead to dma_free_attrs() being called twice on the same pointer? > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_init_cmdlist() { > [ ... ] > node = kcalloc(G2D_CMDLIST_NUM, sizeof(*node), GFP_KERNEL); > [ ... ] > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_fini_cmdlist() { > [ ... ] > kfree(g2d->cmdlist_node); > [ ... ] [Severity: Medium] This is a pre-existing issue, but the allocated array is assigned to the local variable 'node' in g2d_init_cmdlist(), and it is never saved to g2d->cmdlist_node. Does this mean kfree(g2d->cmdlist_node) in g2d_fini_cmdlist() is always called on a NULL pointer, permanently leaking the 64 allocated nodes? > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_close() { > [ ... ] > mutex_lock(&g2d->cmdlist_mutex); > list_for_each_entry_safe(node, n, &g2d_priv->inuse_cmdlist, list) { > [ ... ] > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_finish_event() { > [ ... ] > list_move_tail(&e->base.link, &e->base.file_priv->event_list); > wake_up_interruptible(&e->base.file_priv->event_wait); > [ ... ] [Severity: Critical] This is a pre-existing issue, but if a user submits an asynchronous ioctl and closes the file descriptor before the hardware finishes, g2d_close() cleans up inuse_cmdlist but doesn't wait for pending jobs in the global runqueue. Later, when the hardware interrupt fires, g2d_finish_event() dereferences e->base.file_priv. Could this access a freed drm_file object? > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_runqueue_worker() { > [ ... ] > if (g2d->runqueue_node->async) > g2d_free_runqueue_node(g2d, g2d->runqueue_node); > > if (g2d->suspended) > g2d->runqueue_node = NULL; > else > g2d_exec_runqueue(g2d); > [ ... ] > drivers/gpu/drm/exynos/exynos_drm_g2d.c:g2d_finish_event() { > [ ... ] > struct g2d_runqueue_node *runqueue_node = g2d->runqueue_node; > [ ... ] [Severity: High] This is a pre-existing issue, but the worker thread calls g2d_free_runqueue_node() while holding a mutex, momentarily leaving g2d->runqueue_node pointing to freed memory until it replaces it inside g2d_exec_runqueue(). If a hardirq preempts the worker thread during this window, could g2d_finish_event() locklessly read and dereference the freed pointer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
