Re: [PATCH] drm/exynos: g2d: fix autosuspend cleanup on probe failure

[sashiko-bot@kernel.org]

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 <lgs201920130244@gmail.com>
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/20260808121752.2819486-1-lgs201920130244@gmail.com?part=1