linux-kernel - Re: [PATCH] drm/exynos: fix race condition UAF in exynos_g2d_exec

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 31 May 2023 10:19:17 +0200
From:   Andi Shyti <andi.shyti@...nel.org>
To:     lm0963 <lm0963hack@...il.com>
Cc:     inki.dae@...sung.com, sw0312.kim@...sung.com,
        kyungmin.park@...sung.com, airlied@...il.com, daniel@...ll.ch,
        krzysztof.kozlowski@...aro.org, alim.akhtar@...sung.com,
        dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/exynos: fix race condition UAF in
 exynos_g2d_exec_ioctl

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@...il.com>
> > > ---
> > >  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.

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.

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

Andi