[PATCH v2] drm/xe: Fix possible UAF in guc_exec_queue_process_msg

[PATCH v2] drm/xe: Fix possible UAF in guc_exec_queue_process_msg

[Matthew Brost]

Store xe_device ahead of processing message as message can be free'd in
some cases.

v2:
 - Including missing local changes

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202407231445.rpisd1vA-lkp@intel.com/
Fixes: d930c19fdff3 ("drm/xe: Build PM into GuC CT layer")
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_submit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index da2ead86b9ae..b8f938539a90 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1395,6 +1395,8 @@ static void __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
 
 static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
 {
+	struct xe_device *xe = guc_to_xe(exec_queue_to_guc(msg->private_data));
+
 	trace_xe_sched_msg_recv(msg);
 
 	switch (msg->opcode) {
@@ -1414,7 +1416,7 @@ static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
 		XE_WARN_ON("Unknown message type");
 	}
 
-	xe_pm_runtime_put(guc_to_xe(exec_queue_to_guc(msg->private_data)));
+	xe_pm_runtime_put(xe);
 }
 
 static const struct drm_sched_backend_ops drm_sched_ops = {
-- 
2.34.1

✓ CI.Patch_applied: success for drm/xe: Fix possible UAF in guc_exec_queue_process_msg (rev2)

[Patchwork]

== Series Details ==

Series: drm/xe: Fix possible UAF in guc_exec_queue_process_msg (rev2)
URL   : https://patchwork.freedesktop.org/series/136397/
State : success

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 8a52c2cc81b4 drm-tip: 2024y-07m-23d-20h-01m-00s UTC integration manifest
=== git am output follows ===
Applying: drm/xe: Fix possible UAF in guc_exec_queue_process_msg

✗ CI.checkpatch: warning for drm/xe: Fix possible UAF in guc_exec_queue_process_msg (rev2)

[Patchwork]

== Series Details ==

Series: drm/xe: Fix possible UAF in guc_exec_queue_process_msg (rev2)
URL   : https://patchwork.freedesktop.org/series/136397/
State : warning

== Summary ==

+ KERNEL=/kernel
+ git clone https://gitlab.freedesktop.org/drm/maintainer-tools mt
Cloning into 'mt'...
warning: redirecting to https://gitlab.freedesktop.org/drm/maintainer-tools.git/
+ git -C mt rev-list -n1 origin/master
5ce3e132caaa5b45e5e50201b574a097d130967c
+ cd /kernel
+ git config --global --add safe.directory /kernel
+ git log -n1
commit a3c7bb198ddd661a528e739b6416610ad982edc8
Author: Matthew Brost <matthew.brost@intel.com>
Date:   Tue Jul 23 12:19:03 2024 -0700

    drm/xe: Fix possible UAF in guc_exec_queue_process_msg
    
    Store xe_device ahead of processing message as message can be free'd in
    some cases.
    
    v2:
     - Including missing local changes
    
    Reported-by: kernel test robot <lkp@intel.com>
    Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
    Closes: https://lore.kernel.org/r/202407231445.rpisd1vA-lkp@intel.com/
    Fixes: d930c19fdff3 ("drm/xe: Build PM into GuC CT layer")
    Signed-off-by: Matthew Brost <matthew.brost@intel.com>
+ /mt/dim checkpatch 8a52c2cc81b4c9cfee85520ec8a9f29cfc319e7e drm-intel
a3c7bb198ddd drm/xe: Fix possible UAF in guc_exec_queue_process_msg
-:12: WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
#12: 
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>

total: 0 errors, 1 warnings, 0 checks, 16 lines checked

✗ CI.KUnit: failure for drm/xe: Fix possible UAF in guc_exec_queue_process_msg (rev2)

[Patchwork]

== Series Details ==

Series: drm/xe: Fix possible UAF in guc_exec_queue_process_msg (rev2)
URL   : https://patchwork.freedesktop.org/series/136397/
State : failure

== Summary ==

+ trap cleanup EXIT
+ /kernel/tools/testing/kunit/kunit.py run --kunitconfig /kernel/drivers/gpu/drm/xe/.kunitconfig
ERROR:root:../drivers/gpu/drm/xe/xe_device.c: In function ‘xe_file_close’:
../drivers/gpu/drm/xe/xe_device.c:158:20: error: ‘xe’ undeclared (first use in this function); did you mean ‘xef’?
  158 |  xe_pm_runtime_get(xe);
      |                    ^~
      |                    xef
../drivers/gpu/drm/xe/xe_device.c:158:20: note: each undeclared identifier is reported only once for each function it appears in
make[7]: *** [../scripts/Makefile.build:244: drivers/gpu/drm/xe/xe_device.o] Error 1
make[7]: *** Waiting for unfinished jobs....
make[6]: *** [../scripts/Makefile.build:485: drivers/gpu/drm/xe] Error 2
make[6]: *** Waiting for unfinished jobs....
make[5]: *** [../scripts/Makefile.build:485: drivers/gpu/drm] Error 2
make[4]: *** [../scripts/Makefile.build:485: drivers/gpu] Error 2
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [../scripts/Makefile.build:485: drivers] Error 2
make[3]: *** Waiting for unfinished jobs....
../lib/iomap.c:156:5: warning: no previous prototype for ‘ioread64_lo_hi’ [-Wmissing-prototypes]
  156 | u64 ioread64_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:163:5: warning: no previous prototype for ‘ioread64_hi_lo’ [-Wmissing-prototypes]
  163 | u64 ioread64_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~
../lib/iomap.c:170:5: warning: no previous prototype for ‘ioread64be_lo_hi’ [-Wmissing-prototypes]
  170 | u64 ioread64be_lo_hi(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:178:5: warning: no previous prototype for ‘ioread64be_hi_lo’ [-Wmissing-prototypes]
  178 | u64 ioread64be_hi_lo(const void __iomem *addr)
      |     ^~~~~~~~~~~~~~~~
../lib/iomap.c:264:6: warning: no previous prototype for ‘iowrite64_lo_hi’ [-Wmissing-prototypes]
  264 | void iowrite64_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:272:6: warning: no previous prototype for ‘iowrite64_hi_lo’ [-Wmissing-prototypes]
  272 | void iowrite64_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~
../lib/iomap.c:280:6: warning: no previous prototype for ‘iowrite64be_lo_hi’ [-Wmissing-prototypes]
  280 | void iowrite64be_lo_hi(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
../lib/iomap.c:288:6: warning: no previous prototype for ‘iowrite64be_hi_lo’ [-Wmissing-prototypes]
  288 | void iowrite64be_hi_lo(u64 val, void __iomem *addr)
      |      ^~~~~~~~~~~~~~~~~
make[2]: *** [/kernel/Makefile:1934: .] Error 2
make[1]: *** [/kernel/Makefile:240: __sub-make] Error 2
make: *** [Makefile:240: __sub-make] Error 2

[20:16:14] Configuring KUnit Kernel ...
Generating .config ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
[20:16:18] Building KUnit Kernel ...
Populating config with:
$ make ARCH=um O=.kunit olddefconfig
Building with:
$ make ARCH=um O=.kunit --jobs=48
+ cleanup
++ stat -c %u:%g /kernel
+ chown -R 1003:1003 /kernel

RE: [PATCH v2] drm/xe: Fix possible UAF in guc_exec_queue_process_msg

[Upadhyay, Tejas]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> Matthew Brost
> Sent: Wednesday, July 24, 2024 12:49 AM
> To: intel-xe@lists.freedesktop.org
> Cc: dan.carpenter@linaro.org
> Subject: [PATCH v2] drm/xe: Fix possible UAF in
> guc_exec_queue_process_msg
> 
> Store xe_device ahead of processing message as message can be free'd in
> some cases.
> 
> v2:
>  - Including missing local changes
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202407231445.rpisd1vA-lkp@intel.com/
> Fixes: d930c19fdff3 ("drm/xe: Build PM into GuC CT layer")
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_submit.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> b/drivers/gpu/drm/xe/xe_guc_submit.c
> index da2ead86b9ae..b8f938539a90 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -1395,6 +1395,8 @@ static void
> __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
> 
>  static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)  {
> +	struct xe_device *xe =
> +guc_to_xe(exec_queue_to_guc(msg->private_data));

If msg freed at this point, don't you need to protect against NULL, just in case?

Thanks,
Tejas
> +
>  	trace_xe_sched_msg_recv(msg);
> 
>  	switch (msg->opcode) {
> @@ -1414,7 +1416,7 @@ static void guc_exec_queue_process_msg(struct
> xe_sched_msg *msg)
>  		XE_WARN_ON("Unknown message type");
>  	}
> 
> -	xe_pm_runtime_put(guc_to_xe(exec_queue_to_guc(msg-
> >private_data)));
> +	xe_pm_runtime_put(xe);
>  }
> 
>  static const struct drm_sched_backend_ops drm_sched_ops = {
> --
> 2.34.1

Re: [PATCH v2] drm/xe: Fix possible UAF in guc_exec_queue_process_msg

[Ghimiray, Himal Prasad]

On 24-07-2024 00:49, Matthew Brost wrote:
> Store xe_device ahead of processing message as message can be free'd in
> some cases.
> 
> v2:
>   - Including missing local changes
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202407231445.rpisd1vA-lkp@intel.com/
> Fixes: d930c19fdff3 ("drm/xe: Build PM into GuC CT layer")
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc_submit.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index da2ead86b9ae..b8f938539a90 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -1395,6 +1395,8 @@ static void __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
>   
>   static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
>   {
> +	struct xe_device *xe = guc_to_xe(exec_queue_to_guc(msg->private_data));
> +
>   	trace_xe_sched_msg_recv(msg);
>   
>   	switch (msg->opcode) {
> @@ -1414,7 +1416,7 @@ static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)
>   		XE_WARN_ON("Unknown message type");
>   	}
>   
> -	xe_pm_runtime_put(guc_to_xe(exec_queue_to_guc(msg->private_data)));
> +	xe_pm_runtime_put(xe);

Patch LGTM.
Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>

>   }
>   
>   static const struct drm_sched_backend_ops drm_sched_ops = {

Re: [PATCH v2] drm/xe: Fix possible UAF in guc_exec_queue_process_msg

[Matthew Brost]

On Wed, Jul 24, 2024 at 09:42:11AM -0600, Upadhyay, Tejas wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > Matthew Brost
> > Sent: Wednesday, July 24, 2024 12:49 AM
> > To: intel-xe@lists.freedesktop.org
> > Cc: dan.carpenter@linaro.org
> > Subject: [PATCH v2] drm/xe: Fix possible UAF in
> > guc_exec_queue_process_msg
> > 
> > Store xe_device ahead of processing message as message can be free'd in
> > some cases.
> > 
> > v2:
> >  - Including missing local changes
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202407231445.rpisd1vA-lkp@intel.com/
> > Fixes: d930c19fdff3 ("drm/xe: Build PM into GuC CT layer")
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_guc_submit.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index da2ead86b9ae..b8f938539a90 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -1395,6 +1395,8 @@ static void
> > __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
> > 
> >  static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)  {
> > +	struct xe_device *xe =
> > +guc_to_xe(exec_queue_to_guc(msg->private_data));
> 
> If msg freed at this point, don't you need to protect against NULL, just in case?
> 

The msg is valid here, it can be freed in the below swicth statement
hence the bug in referencing it after the switch statement.

Matt

> Thanks,
> Tejas
> > +
> >  	trace_xe_sched_msg_recv(msg);
> > 
> >  	switch (msg->opcode) {
> > @@ -1414,7 +1416,7 @@ static void guc_exec_queue_process_msg(struct
> > xe_sched_msg *msg)
> >  		XE_WARN_ON("Unknown message type");
> >  	}
> > 
> > -	xe_pm_runtime_put(guc_to_xe(exec_queue_to_guc(msg-
> > >private_data)));
> > +	xe_pm_runtime_put(xe);
> >  }
> > 
> >  static const struct drm_sched_backend_ops drm_sched_ops = {
> > --
> > 2.34.1
> 

Re: [PATCH v2] drm/xe: Fix possible UAF in guc_exec_queue_process_msg

[Ghimiray, Himal Prasad]

On 24-07-2024 21:12, Upadhyay, Tejas wrote:
> 
> 
>> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
>> Matthew Brost
>> Sent: Wednesday, July 24, 2024 12:49 AM
>> To: intel-xe@lists.freedesktop.org
>> Cc: dan.carpenter@linaro.org
>> Subject: [PATCH v2] drm/xe: Fix possible UAF in
>> guc_exec_queue_process_msg
>>
>> Store xe_device ahead of processing message as message can be free'd in
>> some cases.
>>
>> v2:
>>   - Including missing local changes
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/202407231445.rpisd1vA-lkp@intel.com/
>> Fixes: d930c19fdff3 ("drm/xe: Build PM into GuC CT layer")
>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_submit.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
>> b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index da2ead86b9ae..b8f938539a90 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -1395,6 +1395,8 @@ static void
>> __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
>>
>>   static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)  {
>> +	struct xe_device *xe =
>> +guc_to_xe(exec_queue_to_guc(msg->private_data));
> 
> If msg freed at this point, don't you need to protect against NULL, just in case?
> 
> Thanks,
> Tejas

xe_sched_process_msg_work the caller of the ops ensures the routine is 
called only if msg is valid.

Below msg->opcode for eg like CLEANUP can clean it uo.

>> +
>>   	trace_xe_sched_msg_recv(msg);
>>
>>   	switch (msg->opcode) {
>> @@ -1414,7 +1416,7 @@ static void guc_exec_queue_process_msg(struct
>> xe_sched_msg *msg)
>>   		XE_WARN_ON("Unknown message type");
>>   	}
>>
>> -	xe_pm_runtime_put(guc_to_xe(exec_queue_to_guc(msg-
>>> private_data)));
>> +	xe_pm_runtime_put(xe);
>>   }
>>
>>   static const struct drm_sched_backend_ops drm_sched_ops = {
>> --
>> 2.34.1
> 

RE: [PATCH v2] drm/xe: Fix possible UAF in guc_exec_queue_process_msg

[Upadhyay, Tejas]

> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com>
> Sent: Wednesday, July 24, 2024 9:14 PM
> To: Upadhyay, Tejas <tejas.upadhyay@intel.com>
> Cc: intel-xe@lists.freedesktop.org; dan.carpenter@linaro.org
> Subject: Re: [PATCH v2] drm/xe: Fix possible UAF in
> guc_exec_queue_process_msg
> 
> On Wed, Jul 24, 2024 at 09:42:11AM -0600, Upadhyay, Tejas wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > > Matthew Brost
> > > Sent: Wednesday, July 24, 2024 12:49 AM
> > > To: intel-xe@lists.freedesktop.org
> > > Cc: dan.carpenter@linaro.org
> > > Subject: [PATCH v2] drm/xe: Fix possible UAF in
> > > guc_exec_queue_process_msg
> > >
> > > Store xe_device ahead of processing message as message can be free'd
> > > in some cases.
> > >
> > > v2:
> > >  - Including missing local changes
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes:
> > > https://lore.kernel.org/r/202407231445.rpisd1vA-lkp@intel.com/
> > > Fixes: d930c19fdff3 ("drm/xe: Build PM into GuC CT layer")
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_submit.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index da2ead86b9ae..b8f938539a90 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > @@ -1395,6 +1395,8 @@ static void
> > > __guc_exec_queue_process_msg_resume(struct xe_sched_msg *msg)
> > >
> > >  static void guc_exec_queue_process_msg(struct xe_sched_msg *msg)  {
> > > +	struct xe_device *xe =
> > > +guc_to_xe(exec_queue_to_guc(msg->private_data));
> >
> > If msg freed at this point, don't you need to protect against NULL, just in
> case?
> >
> 
> The msg is valid here, it can be freed in the below swicth statement hence the
> bug in referencing it after the switch statement.

Ok, LGTM,
Acked-by: Tejas Upadhyay <tejas.upadhyay@intel.com>

Tejas
> 
> Matt
> 
> > Thanks,
> > Tejas
> > > +
> > >  	trace_xe_sched_msg_recv(msg);
> > >
> > >  	switch (msg->opcode) {
> > > @@ -1414,7 +1416,7 @@ static void guc_exec_queue_process_msg(struct
> > > xe_sched_msg *msg)
> > >  		XE_WARN_ON("Unknown message type");
> > >  	}
> > >
> > > -	xe_pm_runtime_put(guc_to_xe(exec_queue_to_guc(msg-
> > > >private_data)));
> > > +	xe_pm_runtime_put(xe);
> > >  }
> > >
> > >  static const struct drm_sched_backend_ops drm_sched_ops = {
> > > --
> > > 2.34.1
> >