Hi Andi, On Wed, May 31, 2023 at 4:19 PM Andi Shyti <andi.shyti@xxxxxxxxxx> wrote: > > Hi Min, > > > > > 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 <lm0963hack@xxxxxxxxx> > > > > --- > > > > 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