Re: [PATCH] drm/exynos: fix race condition UAF in exynos_g2d_exec_ioctl

Hi Andi,

On Wed, May 31, 2023 at 4:19 PM Andi Shyti [off-list ref] wrote:

Hi Min,

quoted

quoted

quoted

If it is async, runqueue_node is freed in g2d_runqueue_worker on another
worker thread. So in extreme cases, if g2d_runqueue_worker runs first, and
then executes the following if statement, there will be use-after-free.

Signed-off-by: Min Li <redacted>
---
 drivers/gpu/drm/exynos/exynos_drm_g2d.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index ec784e58da5c..414e585ec7dd 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c

@@ -1335,7 +1335,7 @@ int exynos_g2d_exec_ioctl(struct drm_device *drm_dev, void *data,
      /* Let the runqueue know that there is work to do. */
      queue_work(g2d->g2d_workq, &g2d->runqueue_work);

-     if (runqueue_node->async)
+     if (req->async)

did you actually hit this? If you did, then the fix is not OK.

No, I didn't actually hit this. I found it through code review. This
is only a theoretical issue that can only be triggered in extreme
cases.

first of all runqueue is used again two lines below this, which
means that if you don't hit the uaf here you will hit it
immediately after.

No, if async is true, then it will goto out, which will directly return.

if (runqueue_node->async)
    goto out;   // here, go to out, will directly return

wait_for_completion(&runqueue_node->complete);      // not hit
g2d_free_runqueue_node(g2d, runqueue_node);

out:
return 0;

Second, if runqueue is freed, than we need to remove the part
where it's freed because it doesn't make sense to free runqueue
at this stage.

It is freed by g2d_free_runqueue_node in g2d_runqueue_worker

static void g2d_runqueue_worker(struct work_struct *work)
{
    ......
    if (runqueue_node) {
        pm_runtime_mark_last_busy(g2d->dev);
        pm_runtime_put_autosuspend(g2d->dev);

        complete(&runqueue_node->complete);
        if (runqueue_node->async)
            g2d_free_runqueue_node(g2d, runqueue_node);        // freed here
    }

Finally, can you elaborate on the code review that you did so
that we all understand it?

queue_work(g2d->g2d_workq, &g2d->runqueue_work);
msleep(100);        // add sleep here to let g2d_runqueue_worker run first
if (runqueue_node->async)
    goto out;

Andi

--
Min Li

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel