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;
From:   =?ks_c_5601-1987?B?tOvAzrHiL1RpemVuIFBsYXRmb3JtIExhYihTUikvu++8usD8wNo=?= 
        <inki.dae@samsung.com>
To:     "'Andi Shyti'" <andi.shyti@kernel.org>
Cc:     "'lm0963'" <lm0963hack@gmail.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>
In-Reply-To: <20230601082928.4mk7hfi5hunaxm4y@intel.intel>
Subject: RE: [PATCH] drm/exynos: fix race condition UAF in
 exynos_g2d_exec_ioctl
Date:   Fri, 2 Jun 2023 10:20:28 +0900
Message-ID: <004e01d994f0$6af0fb20$40d2f160$@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="ks_c_5601-1987"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQGTj1Ryyv7UvGCwDbo/bVuHx7F6MwGboBvXA23aa5ECFYZTkQHsjiyBAkvDF18C1e4e+AKZUix2AY4xbwKvcBbQwA==
Content-Language: ko
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20230526130131.16521-1-lm0963hack@gmail.com>
        <20230530222150.24oogloda6wtvpvm@intel.intel>
        <CAAgLYK7FNrAYFRp7C3LDtqevFENQLw8YYAFR2Pk9wdfQ5RKVeg@mail.gmail.com>
        <20230531081917.grx3qqqm7usaqoa5@intel.intel>
        <CAAgLYK4KzuKMTkBwzCJj4wa+W+=9fR+A1J=XR-n1E-W7EjexuA@mail.gmail.com>
        <CGME20230531120656epcas1p1df61dae784f4b6833b39c43da697be08@epcas1p1.samsung.com>
        <20230531120531.dynd3iyggauucly7@intel.intel>
        <007001d99413$0f08ba60$2d1a2f20$@samsung.com>
        <20230601082928.4mk7hfi5hunaxm4y@intel.intel>
Precedence: bulk

Andi=7E :)

> -----Original Message-----
> From: Andi Shyti <andi.shyti=40kernel.org>
> Sent: Thursday, June 1, 2023 5:29 PM
> To: =B4=EB=C0=CE=B1=E2/Tizen=20Platform=20Lab(SR)/=BB=EF=BC=BA=C0=FC=C0=
=DA=20<inki.dae=40samsung.com>=0D=0A>=20Cc:=20'lm0963'=20<lm0963hack=40gmai=
l.com>;=20sw0312.kim=40samsung.com;=0D=0A>=20kyungmin.park=40samsung.com;=
=20airlied=40gmail.com;=20daniel=40ffwll.ch;=0D=0A>=20krzysztof.kozlowski=
=40linaro.org;=20alim.akhtar=40samsung.com;=20dri-=0D=0A>=20devel=40lists.f=
reedesktop.org;=20linux-arm-kernel=40lists.infradead.org;=20linux-=0D=0A>=
=20samsung-soc=40vger.kernel.org;=20linux-kernel=40vger.kernel.org=0D=0A>=
=20Subject:=20Re:=20=5BPATCH=5D=20drm/exynos:=20fix=20race=20condition=20UA=
F=20in=0D=0A>=20exynos_g2d_exec_ioctl=0D=0A>=20=0D=0A>=20Hi=20Inki,=0D=0A>=
=20=0D=0A>=20>=20>=20>=20>=20>=20>=20>=20If=20it=20is=20async,=20runqueue_n=
ode=20is=20freed=20in=0D=0A>=20g2d_runqueue_worker=20on=0D=0A>=20>=20>=20an=
other=0D=0A>=20>=20>=20>=20>=20>=20>=20>=20worker=20thread.=20So=20in=20ext=
reme=20cases,=20if=20g2d_runqueue_worker=0D=0A>=20runs=0D=0A>=20>=20>=20fir=
st,=20and=0D=0A>=20>=20>=20>=20>=20>=20>=20>=20then=20executes=20the=20foll=
owing=20if=20statement,=20there=20will=20be=20use-=0D=0A>=20>=20>=20after-f=
ree.=0D=0A>=20>=20>=20>=20>=20>=20>=20>=0D=0A>=20>=20>=20>=20>=20>=20>=20>=
=20Signed-off-by:=20Min=20Li=20<lm0963hack=40gmail.com>=0D=0A>=20>=20>=20>=
=20>=20>=20>=20>=20---=0D=0A>=20>=20>=20>=20>=20>=20>=20>=20=20drivers/gpu/=
drm/exynos/exynos_drm_g2d.c=20=7C=202=20+-=0D=0A>=20>=20>=20>=20>=20>=20>=
=20>=20=201=20file=20changed,=201=20insertion(+),=201=20deletion(-)=0D=0A>=
=20>=20>=20>=20>=20>=20>=20>=0D=0A>=20>=20>=20>=20>=20>=20>=20>=20diff=20--=
git=20a/drivers/gpu/drm/exynos/exynos_drm_g2d.c=0D=0A>=20>=20>=20b/drivers/=
gpu/drm/exynos/exynos_drm_g2d.c=0D=0A>=20>=20>=20>=20>=20>=20>=20>=20index=
=20ec784e58da5c..414e585ec7dd=20100644=0D=0A>=20>=20>=20>=20>=20>=20>=20>=
=20---=20a/drivers/gpu/drm/exynos/exynos_drm_g2d.c=0D=0A>=20>=20>=20>=20>=
=20>=20>=20>=20+++=20b/drivers/gpu/drm/exynos/exynos_drm_g2d.c=0D=0A>=20>=
=20>=20>=20>=20>=20>=20>=20=40=40=20-1335,7=20+1335,7=20=40=40=20int=20exyn=
os_g2d_exec_ioctl(struct=0D=0A>=20>=20>=20drm_device=20*drm_dev,=20void=20*=
data,=0D=0A>=20>=20>=20>=20>=20>=20>=20>=20=20=20=20=20=20=20/*=20Let=20the=
=20runqueue=20know=20that=20there=20is=20work=20to=20do.=20*/=0D=0A>=20>=20=
>=20>=20>=20>=20>=20>=20=20=20=20=20=20=20queue_work(g2d->g2d_workq,=20&g2d=
->runqueue_work);=0D=0A>=20>=20>=20>=20>=20>=20>=20>=0D=0A>=20>=20>=20>=20>=
=20>=20>=20>=20-=20=20=20=20=20if=20(runqueue_node->async)=0D=0A>=20>=20>=
=20>=20>=20>=20>=20>=20+=20=20=20=20=20if=20(req->async)=0D=0A>=20>=20>=20>=
=20>=20>=20>=0D=0A>=20>=20>=20>=20>=20>=20>=20did=20you=20actually=20hit=20=
this?=20If=20you=20did,=20then=20the=20fix=20is=20not=20OK.=0D=0A>=20>=20>=
=20>=20>=20>=0D=0A>=20>=20>=20>=20>=20>=20No,=20I=20didn't=20actually=20hit=
=20this.=20I=20found=20it=20through=20code=20review.=0D=0A>=20This=0D=0A>=
=20>=20>=20>=20>=20>=20is=20only=20a=20theoretical=20issue=20that=20can=20o=
nly=20be=20triggered=20in=0D=0A>=20extreme=0D=0A>=20>=20>=20>=20>=20>=20cas=
es.=0D=0A>=20>=20>=20>=20>=0D=0A>=20>=20>=20>=20>=20first=20of=20all=20runq=
ueue=20is=20used=20again=20two=20lines=20below=20this,=20which=0D=0A>=20>=
=20>=20>=20>=20means=20that=20if=20you=20don't=20hit=20the=20uaf=20here=20y=
ou=20will=20hit=20it=0D=0A>=20>=20>=20>=20>=20immediately=20after.=0D=0A>=
=20>=20>=20>=0D=0A>=20>=20>=20>=20No,=20if=20async=20is=20true,=20then=20it=
=20will=20goto=20out,=20which=20will=20directly=0D=0A>=20return.=0D=0A>=20>=
=20>=20>=0D=0A>=20>=20>=20>=20if=20(runqueue_node->async)=0D=0A>=20>=20>=20=
>=20=20=20=20=20goto=20out;=20=20=20//=20here,=20go=20to=20out,=20will=20di=
rectly=20return=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=20>=20wait_for_completion=
(&runqueue_node->complete);=20=20=20=20=20=20//=20not=20hit=0D=0A>=20>=20>=
=20>=20g2d_free_runqueue_node(g2d,=20runqueue_node);=0D=0A>=20>=20>=20>=0D=
=0A>=20>=20>=20>=20out:=0D=0A>=20>=20>=20>=20return=200;=0D=0A>=20>=20>=0D=
=0A>=20>=20>=20that's=20right,=20sorry,=20I=20misread=20it.=0D=0A>=20>=20>=
=0D=0A>=20>=20>=20>=20>=20Second,=20if=20runqueue=20is=20freed,=20than=20we=
=20need=20to=20remove=20the=20part=0D=0A>=20>=20>=20>=20>=20where=20it's=20=
freed=20because=20it=20doesn't=20make=20sense=20to=20free=20runqueue=0D=0A>=
=20>=20>=20>=20>=20at=20this=20stage.=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=20>=
=20It=20is=20freed=20by=20g2d_free_runqueue_node=20in=20g2d_runqueue_worker=
=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=20>=20static=20void=20g2d_runqueue_worke=
r(struct=20work_struct=20*work)=0D=0A>=20>=20>=20>=20=7B=0D=0A>=20>=20>=20>=
=20=20=20=20=20......=0D=0A>=20>=20>=20>=20=20=20=20=20if=20(runqueue_node)=
=20=7B=0D=0A>=20>=20>=20>=20=20=20=20=20=20=20=20=20pm_runtime_mark_last_bu=
sy(g2d->dev);=0D=0A>=20>=20>=20>=20=20=20=20=20=20=20=20=20pm_runtime_put_a=
utosuspend(g2d->dev);=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=20>=20=20=20=20=20=
=20=20=20=20complete(&runqueue_node->complete);=0D=0A>=20>=20>=20>=20=20=20=
=20=20=20=20=20=20if=20(runqueue_node->async)=0D=0A>=20>=20>=20>=20=20=20=
=20=20=20=20=20=20=20=20=20=20g2d_free_runqueue_node(g2d,=20runqueue_node);=
=20=20=20=20=20=20=20=20//=0D=0Afreed=0D=0A>=20here=0D=0A>=20>=20>=0D=0A>=
=20>=20>=20this=20is=20what=20I'm=20wondering:=20is=20it=20correct=20to=20f=
ree=20a=20resource=0D=0A>=20>=20>=20here?=20The=20design=20looks=20to=20me=
=20a=20bit=20fragile=20and=20prone=20to=20mistakes.=0D=0A>=20>=0D=0A>=20>=
=20This=20question=20seems=20to=20deviate=20from=20the=20purpose=20of=20thi=
s=20patch.=20If=20you=0D=0A>=20are=20providing=20additional=20opinions=20fo=
r=20code=20quality=20improvement=20unrelated=0D=0A>=20to=20this=20patch,=20=
it=20would=20be=20more=20appropriate=20for=20me=20to=20answer=20instead=20o=
f=0D=0A>=20him.=0D=0A>=20=0D=0A>=20It's=20not=20deviating=20as=20the=20ques=
tion=20was=20already=20made=20in=20my=20first=0D=0A>=20review.=20It=20just=
=20looks=20strange=20to=20me=20that=20a=20piece=20of=20data=20shared=0D=0A>=
=20amongst=20processes=20can=20be=20freed=20up=20without=20sinchronizing.=
=20A=20bunch=0D=0A=0D=0AI=20believe=20that=20if=20we=20overlook=20any=20dou=
bts=20or=20concerns=20about=20worrisome=0D=0Aaspects=20without=20completely=
=20resolving=20them,=20it=20wouldn't=20be=20helpful=20to=20the=0D=0Acommuni=
ty.=0D=0ATherefore,=20I=20would=20like=20to=20clarify=20more=20explicitly=
=20in=20order=20to=20ensure=20a=0D=0Abetter=20understanding.=0D=0A=0D=0AAFA=
IK,=20the=20data=20you=20mentioned=20isn't=20shared=20between=20processes.=
=20This=20data=20is=0D=0Agenerated=20driver-internally=20when=20the=20user=
=20makes=20a=20rendering=20request=20and=0D=0Awill=20be=20removed=20once=20=
the=202D=20GPU=20finishes=20rendering.=0D=0A=0D=0A=0D=0AHowever,=20there=20=
may=20be=20another=20issue=20that=20I'm=20not=20aware=20of,=20so=20if=20the=
re=20is=0D=0Aany,=20give=20me=20it=20more=20specifically=20as=20it=20would=
=20help=20improve=20driver=20stability.=0D=0A=0D=0AThanks=20again,=0D=0AInk=
i=20Dae=0D=0A=0D=0A>=20of=20if's=20do=20not=20make=20it=20robust=20enough.=
=0D=0A>=20=0D=0A>=20The=20patch=20itself,=20in=20my=20point=20of=20view,=20=
is=20not=20really=20fixing=20much=0D=0A>=20and=20won't=20make=20any=20diffe=
rence,=20it's=20just=20exposing=20the=20weakness=20I=0D=0A>=20mentioned.=0D=
=0A>=20=0D=0A>=20However,=20honestly=20speaking,=20I=20don't=20know=20the=
=20driver=20well=20enough=0D=0A>=20to=20suggest=20architectural=20changes=
=20and=20that's=20why=20I=20r-b'ed=20this=0D=0A>=20one.=20But=20the=20first=
=20thing=20that=20comes=20to=20my=20mind,=20without=20looking=0D=0A>=20much=
=20at=20the=20code,=20is=20using=20kref's=20as=20a=20way=20to=20make=20sure=
=20that=20a=0D=0A>=20resource=20doesn't=20magically=20disappear=20under=20y=
our=20nose.=0D=0A>=20=0D=0A>=20But,=20of=20course,=20this=20is=20up=20to=20=
you=20and=20if=20in=20your=20opinion=20this=20is=0D=0A>=20OK=20and=20it=20f=
ixes=20it...=20then=20you=20definitely=20know=20better=20:)=0D=0A>=20=0D=0A=
>=20Thanks=20for=20this=20discussion,=0D=0A>=20Andi=0D=0A>=20=0D=0A>=20>=20=
The=20runqueue=20node=20-=20which=20contains=20command=20list=20for=20g2d=
=20rendering=20-=20is=0D=0A>=20generated=20when=20the=20user=20calls=20the=
=20ioctl=20system=20call.=20Therefore,=20if=20the=0D=0A>=20user-requested=
=20command=20list=20is=20rendered=20by=20g2d=20device=20then=20there=20is=
=20no=0D=0A>=20longer=20a=20reason=20to=20keep=20it.=20:)=0D=0A>=20>=0D=0A>=
=20>=20>=0D=0A>=20>=20>=20The=20patch=20per=20se=20is=20OK.=20It=20doesn't=
=20make=20much=20difference=20to=20me=0D=0A>=20>=20>=20where=20you=20actual=
ly=20read=20async,=20although=20this=20patch=20looks=20a=20bit=0D=0A>=20>=
=20>=20safer:=0D=0A>=20>=20>=0D=0A>=20>=20>=20Reviewed-by:=20Andi=20Shyti=
=20<andi.shyti=40kernel.org>=0D=0A>=20>=0D=0A>=20>=20Thanks,=0D=0A>=20>=20I=
nki=20Dae=0D=0A>=20>=0D=0A>=20>=20>=0D=0A>=20>=20>=20However=20some=20refac=
toring=20might=20be=20needed=20to=20make=20it=20a=20bit=20more=0D=0A>=20>=
=20>=20robust.=0D=0A>=20>=20>=0D=0A>=20>=20>=20Thanks,=0D=0A>=20>=20>=20And=
i=0D=0A>=20>=20>=0D=0A>=20>=20>=20>=20=20=20=20=20=7D=0D=0A>=20>=20>=20>=0D=
=0A>=20>=20>=20>=20>=0D=0A>=20>=20>=20>=20>=20Finally,=20can=20you=20elabor=
ate=20on=20the=20code=20review=20that=20you=20did=20so=0D=0A>=20>=20>=20>=
=20>=20that=20we=20all=20understand=20it?=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=
=20>=20queue_work(g2d->g2d_workq,=20&g2d->runqueue_work);=0D=0A>=20>=20>=20=
>=20msleep(100);=20=20=20=20=20=20=20=20//=20add=20sleep=20here=20to=20let=
=20g2d_runqueue_worker=20run=0D=0A>=20first=0D=0A>=20>=20>=20>=20if=20(runq=
ueue_node->async)=0D=0A>=20>=20>=20>=20=20=20=20=20goto=20out;=0D=0A>=20>=
=20>=20>=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=20>=20>=0D=0A>=20>=20>=20>=20>=
=20Andi=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=20>=0D=0A>=20>=20>=20>=0D=0A>=20>=
=20>=20>=20--=0D=0A>=20>=20>=20>=20Min=20Li=0D=0A>=20>=0D=0A>=20>=0D=0A=0D=
=0A