Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 31 May 2023 10:19:17 +0200
From:   Andi Shyti <andi.shyti@kernel.org>
To:     lm0963 <lm0963hack@gmail.com>
Cc:     inki.dae@samsung.com, sw0312.kim@samsung.com,
        kyungmin.park@samsung.com, airlied@gmail.com, daniel@ffwll.ch,
        krzysztof.kozlowski@linaro.org, alim.akhtar@samsung.com,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/exynos: fix race condition UAF in
 exynos_g2d_exec_ioctl
Message-ID: <20230531081917.grx3qqqm7usaqoa5@intel.intel>
References: <20230526130131.16521-1-lm0963hack@gmail.com>
 <20230530222150.24oogloda6wtvpvm@intel.intel>
 <CAAgLYK7FNrAYFRp7C3LDtqevFENQLw8YYAFR2Pk9wdfQ5RKVeg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAgLYK7FNrAYFRp7C3LDtqevFENQLw8YYAFR2Pk9wdfQ5RKVeg@mail.gmail.com>
Precedence: bulk

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@gmail.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