[PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Thomas Hellström]

The commit
afd2627f727b ("tracing: Check "%s" dereference via the field and not the TP_printk format")
exposes potential UAFs in the xe_bo_move trace event.

Fix those by avoiding dereferencing the
xe_mem_type_to_name[] array at TP_printk time.

Since some code refactoring has taken place, explicit backporting may
be needed for kernels older than 6.10.

Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma traces")
Cc: Gustavo Sousa <gustavo.sousa@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-xe@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.11+
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h b/drivers/gpu/drm/xe/xe_trace_bo.h
index 1762dd30ba6d..ea50fee50c7d 100644
--- a/drivers/gpu/drm/xe/xe_trace_bo.h
+++ b/drivers/gpu/drm/xe/xe_trace_bo.h
@@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
 	    TP_STRUCT__entry(
 		     __field(struct xe_bo *, bo)
 		     __field(size_t, size)
-		     __field(u32, new_placement)
-		     __field(u32, old_placement)
+		     __string(new_placement_name, xe_mem_type_to_name[new_placement])
+		     __string(old_placement_name, xe_mem_type_to_name[old_placement])
 		     __string(device_id, __dev_name_bo(bo))
 		     __field(bool, move_lacks_source)
 			),
@@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
 	    TP_fast_assign(
 		   __entry->bo      = bo;
 		   __entry->size = bo->size;
-		   __entry->new_placement = new_placement;
-		   __entry->old_placement = old_placement;
+		   __assign_str(new_placement_name);
+		   __assign_str(old_placement_name);
 		   __assign_str(device_id);
 		   __entry->move_lacks_source = move_lacks_source;
 		   ),
 	    TP_printk("move_lacks_source:%s, migrate object %p [size %zu] from %s to %s device_id:%s",
 		      __entry->move_lacks_source ? "yes" : "no", __entry->bo, __entry->size,
-		      xe_mem_type_to_name[__entry->old_placement],
-		      xe_mem_type_to_name[__entry->new_placement], __get_str(device_id))
+		      __get_str(old_placement_name),
+		      __get_str(new_placement_name), __get_str(device_id))
 );
 
 DECLARE_EVENT_CLASS(xe_vma,
-- 
2.47.1

Re: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Matthew Brost]

On Mon, Dec 23, 2024 at 02:42:50PM +0100, Thomas Hellström wrote:
> The commit
> afd2627f727b ("tracing: Check "%s" dereference via the field and not the TP_printk format")
> exposes potential UAFs in the xe_bo_move trace event.
> 
> Fix those by avoiding dereferencing the
> xe_mem_type_to_name[] array at TP_printk time.
> 
> Since some code refactoring has taken place, explicit backporting may
> be needed for kernels older than 6.10.
> 
> Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma traces")
> Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-xe@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.11+
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h b/drivers/gpu/drm/xe/xe_trace_bo.h
> index 1762dd30ba6d..ea50fee50c7d 100644
> --- a/drivers/gpu/drm/xe/xe_trace_bo.h
> +++ b/drivers/gpu/drm/xe/xe_trace_bo.h
> @@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
>  	    TP_STRUCT__entry(
>  		     __field(struct xe_bo *, bo)
>  		     __field(size_t, size)
> -		     __field(u32, new_placement)
> -		     __field(u32, old_placement)
> +		     __string(new_placement_name, xe_mem_type_to_name[new_placement])
> +		     __string(old_placement_name, xe_mem_type_to_name[old_placement])
>  		     __string(device_id, __dev_name_bo(bo))
>  		     __field(bool, move_lacks_source)
>  			),
> @@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
>  	    TP_fast_assign(
>  		   __entry->bo      = bo;
>  		   __entry->size = bo->size;
> -		   __entry->new_placement = new_placement;
> -		   __entry->old_placement = old_placement;
> +		   __assign_str(new_placement_name);
> +		   __assign_str(old_placement_name);
>  		   __assign_str(device_id);
>  		   __entry->move_lacks_source = move_lacks_source;
>  		   ),
>  	    TP_printk("move_lacks_source:%s, migrate object %p [size %zu] from %s to %s device_id:%s",
>  		      __entry->move_lacks_source ? "yes" : "no", __entry->bo, __entry->size,
> -		      xe_mem_type_to_name[__entry->old_placement],
> -		      xe_mem_type_to_name[__entry->new_placement], __get_str(device_id))

So is this the UAF? i.e., The Xe module unloads and xe_mem_type_to_name
is gone?

I noticed that xe_mem_type_to_name is not static, it likely should be.
Would that help here?

Matt

> +		      __get_str(old_placement_name),
> +		      __get_str(new_placement_name), __get_str(device_id))
>  );
>  
>  DECLARE_EVENT_CLASS(xe_vma,
> -- 
> 2.47.1
> 

RE: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Thomas Hellström
Sent: Monday, December 23, 2024 5:43 AM
To: intel-xe@lists.freedesktop.org
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Sousa, Gustavo <gustavo.sousa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Radhakrishna Sripada <radhakrishna.sripada@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; stable@vger.kernel.org
Subject: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF
> 
> The commit
> afd2627f727b ("tracing: Check "%s" dereference via the field and not the TP_printk format")
> exposes potential UAFs in the xe_bo_move trace event.
> 
> Fix those by avoiding dereferencing the
> xe_mem_type_to_name[] array at TP_printk time.
> 
> Since some code refactoring has taken place, explicit backporting may
> be needed for kernels older than 6.10.
> 
> Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma traces")
> Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-xe@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.11+
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

I take it we're hitting the WARN_ONCE in ignore_event due to a test_safe_str failure?

I don't know about us hitting a UAF here, but this fix is exactly what was recommended
in the comment immediately above the WARN_ONCE that we shouldn't be hitting, so
this is probably correct if that's what we're trying to avoid.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
-Jonathan Cavitt

> ---
>  drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h b/drivers/gpu/drm/xe/xe_trace_bo.h
> index 1762dd30ba6d..ea50fee50c7d 100644
> --- a/drivers/gpu/drm/xe/xe_trace_bo.h
> +++ b/drivers/gpu/drm/xe/xe_trace_bo.h
> @@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
>  	    TP_STRUCT__entry(
>  		     __field(struct xe_bo *, bo)
>  		     __field(size_t, size)
> -		     __field(u32, new_placement)
> -		     __field(u32, old_placement)
> +		     __string(new_placement_name, xe_mem_type_to_name[new_placement])
> +		     __string(old_placement_name, xe_mem_type_to_name[old_placement])
>  		     __string(device_id, __dev_name_bo(bo))
>  		     __field(bool, move_lacks_source)
>  			),
> @@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
>  	    TP_fast_assign(
>  		   __entry->bo      = bo;
>  		   __entry->size = bo->size;
> -		   __entry->new_placement = new_placement;
> -		   __entry->old_placement = old_placement;
> +		   __assign_str(new_placement_name);
> +		   __assign_str(old_placement_name);
>  		   __assign_str(device_id);
>  		   __entry->move_lacks_source = move_lacks_source;
>  		   ),
>  	    TP_printk("move_lacks_source:%s, migrate object %p [size %zu] from %s to %s device_id:%s",
>  		      __entry->move_lacks_source ? "yes" : "no", __entry->bo, __entry->size,
> -		      xe_mem_type_to_name[__entry->old_placement],
> -		      xe_mem_type_to_name[__entry->new_placement], __get_str(device_id))
> +		      __get_str(old_placement_name),
> +		      __get_str(new_placement_name), __get_str(device_id))
>  );
>  
>  DECLARE_EVENT_CLASS(xe_vma,
> -- 
> 2.47.1
> 
> 

Re: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Thomas Hellström]

On Mon, 2024-12-23 at 15:44 +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> Thomas Hellström
> Sent: Monday, December 23, 2024 5:43 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Sousa,
> Gustavo <gustavo.sousa@intel.com>; De Marchi, Lucas
> <lucas.demarchi@intel.com>; Radhakrishna Sripada
> <radhakrishna.sripada@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> stable@vger.kernel.org
> Subject: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF
> > 
> > The commit
> > afd2627f727b ("tracing: Check "%s" dereference via the field and
> > not the TP_printk format")
> > exposes potential UAFs in the xe_bo_move trace event.
> > 
> > Fix those by avoiding dereferencing the
> > xe_mem_type_to_name[] array at TP_printk time.
> > 
> > Since some code refactoring has taken place, explicit backporting
> > may
> > be needed for kernels older than 6.10.
> > 
> > Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma traces")
> > Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: intel-xe@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v6.11+
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> 
> I take it we're hitting the WARN_ONCE in ignore_event due to a
> test_safe_str failure?

Actually it's the WARN_ONCE in test_event_printk()

if (WARN_ON_ONCE(dereference_flags)) {


> I don't know about us hitting a UAF here, but this fix is exactly
> what was recommended
> in the comment immediately above the WARN_ONCE that we shouldn't be
> hitting, so
> this is probably correct if that's what we're trying to avoid.

I'll double-check to see if I can easily trigger the UAF.


> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

Thanks,
Thomas


> -Jonathan Cavitt
> 
> > ---
> >  drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h
> > b/drivers/gpu/drm/xe/xe_trace_bo.h
> > index 1762dd30ba6d..ea50fee50c7d 100644
> > --- a/drivers/gpu/drm/xe/xe_trace_bo.h
> > +++ b/drivers/gpu/drm/xe/xe_trace_bo.h
> > @@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
> >  	    TP_STRUCT__entry(
> >  		     __field(struct xe_bo *, bo)
> >  		     __field(size_t, size)
> > -		     __field(u32, new_placement)
> > -		     __field(u32, old_placement)
> > +		     __string(new_placement_name,
> > xe_mem_type_to_name[new_placement])
> > +		     __string(old_placement_name,
> > xe_mem_type_to_name[old_placement])
> >  		     __string(device_id, __dev_name_bo(bo))
> >  		     __field(bool, move_lacks_source)
> >  			),
> > @@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
> >  	    TP_fast_assign(
> >  		   __entry->bo      = bo;
> >  		   __entry->size = bo->size;
> > -		   __entry->new_placement = new_placement;
> > -		   __entry->old_placement = old_placement;
> > +		   __assign_str(new_placement_name);
> > +		   __assign_str(old_placement_name);
> >  		   __assign_str(device_id);
> >  		   __entry->move_lacks_source = move_lacks_source;
> >  		   ),
> >  	    TP_printk("move_lacks_source:%s, migrate object %p
> > [size %zu] from %s to %s device_id:%s",
> >  		      __entry->move_lacks_source ? "yes" : "no",
> > __entry->bo, __entry->size,
> > -		      xe_mem_type_to_name[__entry->old_placement],
> > -		      xe_mem_type_to_name[__entry->new_placement],
> > __get_str(device_id))
> > +		      __get_str(old_placement_name),
> > +		      __get_str(new_placement_name),
> > __get_str(device_id))
> >  );
> >  
> >  DECLARE_EVENT_CLASS(xe_vma,
> > -- 
> > 2.47.1
> > 
> > 

Re: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Thomas Hellström]

On Mon, 2024-12-23 at 07:44 -0800, Matthew Brost wrote:
> On Mon, Dec 23, 2024 at 02:42:50PM +0100, Thomas Hellström wrote:
> > The commit
> > afd2627f727b ("tracing: Check "%s" dereference via the field and
> > not the TP_printk format")
> > exposes potential UAFs in the xe_bo_move trace event.
> > 
> > Fix those by avoiding dereferencing the
> > xe_mem_type_to_name[] array at TP_printk time.
> > 
> > Since some code refactoring has taken place, explicit backporting
> > may
> > be needed for kernels older than 6.10.
> > 
> > Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma traces")
> > Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: intel-xe@lists.freedesktop.org
> > Cc: <stable@vger.kernel.org> # v6.11+
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h
> > b/drivers/gpu/drm/xe/xe_trace_bo.h
> > index 1762dd30ba6d..ea50fee50c7d 100644
> > --- a/drivers/gpu/drm/xe/xe_trace_bo.h
> > +++ b/drivers/gpu/drm/xe/xe_trace_bo.h
> > @@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
> >  	    TP_STRUCT__entry(
> >  		     __field(struct xe_bo *, bo)
> >  		     __field(size_t, size)
> > -		     __field(u32, new_placement)
> > -		     __field(u32, old_placement)
> > +		     __string(new_placement_name,
> > xe_mem_type_to_name[new_placement])
> > +		     __string(old_placement_name,
> > xe_mem_type_to_name[old_placement])
> >  		     __string(device_id, __dev_name_bo(bo))
> >  		     __field(bool, move_lacks_source)
> >  			),
> > @@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
> >  	    TP_fast_assign(
> >  		   __entry->bo      = bo;
> >  		   __entry->size = bo->size;
> > -		   __entry->new_placement = new_placement;
> > -		   __entry->old_placement = old_placement;
> > +		   __assign_str(new_placement_name);
> > +		   __assign_str(old_placement_name);
> >  		   __assign_str(device_id);
> >  		   __entry->move_lacks_source = move_lacks_source;
> >  		   ),
> >  	    TP_printk("move_lacks_source:%s, migrate object %p
> > [size %zu] from %s to %s device_id:%s",
> >  		      __entry->move_lacks_source ? "yes" : "no",
> > __entry->bo, __entry->size,
> > -		      xe_mem_type_to_name[__entry->old_placement],
> > -		      xe_mem_type_to_name[__entry->new_placement],
> > __get_str(device_id))
> 
> So is this the UAF? i.e., The Xe module unloads and
> xe_mem_type_to_name
> is gone?

I would imagine that's the intention of the warning. However removing
the xe_module seems to empty the trace buffer of xe_bo_move events.
Whether there is a race in that process or whether the TP_printk check
can't distinguish between module local addresses and other addresses is
hard to tell. In any case, it looks like we need to comply with the
warning here (I suspect CI refuses to run otherwise), and since it only
appears to trigger on the xe module load on my system, it's unlikely
that mentioned commit will be reverted.


> 
> I noticed that xe_mem_type_to_name is not static, it likely should
> be.
> Would that help here?

No, doesn't help unfortunately.

/Thomas



> 
> Matt
> 
> > +		      __get_str(old_placement_name),
> > +		      __get_str(new_placement_name),
> > __get_str(device_id))
> >  );
> >  
> >  DECLARE_EVENT_CLASS(xe_vma,
> > -- 
> > 2.47.1
> > 

RE: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Cavitt, Jonathan]

-----Original Message-----
From: Thomas Hellström <thomas.hellstrom@linux.intel.com> 
Sent: Monday, December 23, 2024 7:57 AM
To: Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-xe@lists.freedesktop.org
Cc: Sousa, Gustavo <gustavo.sousa@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Radhakrishna Sripada <radhakrishna.sripada@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>; stable@vger.kernel.org
Subject: Re: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF
> 
> On Mon, 2024-12-23 at 15:44 +0000, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
> > Thomas Hellström
> > Sent: Monday, December 23, 2024 5:43 AM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Sousa,
> > Gustavo <gustavo.sousa@intel.com>; De Marchi, Lucas
> > <lucas.demarchi@intel.com>; Radhakrishna Sripada
> > <radhakrishna.sripada@intel.com>; Roper, Matthew D
> > <matthew.d.roper@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> > stable@vger.kernel.org
> > Subject: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF
> > > 
> > > The commit
> > > afd2627f727b ("tracing: Check "%s" dereference via the field and
> > > not the TP_printk format")
> > > exposes potential UAFs in the xe_bo_move trace event.
> > > 
> > > Fix those by avoiding dereferencing the
> > > xe_mem_type_to_name[] array at TP_printk time.
> > > 
> > > Since some code refactoring has taken place, explicit backporting
> > > may
> > > be needed for kernels older than 6.10.
> > > 
> > > Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma traces")
> > > Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: intel-xe@lists.freedesktop.org
> > > Cc: <stable@vger.kernel.org> # v6.11+
> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > 
> > I take it we're hitting the WARN_ONCE in ignore_event due to a
> > test_safe_str failure?
> 
> Actually it's the WARN_ONCE in test_event_printk()
> 
> if (WARN_ON_ONCE(dereference_flags)) {

Ah, I see.

There's a comment above that WARN_ON_ONCE as well, and it
more or less recommends the same actions, albeit with less
specificity.  My RB still stands.
-Jonathan Cavitt

> 
> 
> > I don't know about us hitting a UAF here, but this fix is exactly
> > what was recommended
> > in the comment immediately above the WARN_ONCE that we shouldn't be
> > hitting, so
> > this is probably correct if that's what we're trying to avoid.
> 
> I'll double-check to see if I can easily trigger the UAF.
> 
> 
> > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> 
> Thanks,
> Thomas
> 
> 
> > -Jonathan Cavitt
> > 
> > > ---
> > >  drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h
> > > b/drivers/gpu/drm/xe/xe_trace_bo.h
> > > index 1762dd30ba6d..ea50fee50c7d 100644
> > > --- a/drivers/gpu/drm/xe/xe_trace_bo.h
> > > +++ b/drivers/gpu/drm/xe/xe_trace_bo.h
> > > @@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
> > >  	    TP_STRUCT__entry(
> > >  		     __field(struct xe_bo *, bo)
> > >  		     __field(size_t, size)
> > > -		     __field(u32, new_placement)
> > > -		     __field(u32, old_placement)
> > > +		     __string(new_placement_name,
> > > xe_mem_type_to_name[new_placement])
> > > +		     __string(old_placement_name,
> > > xe_mem_type_to_name[old_placement])
> > >  		     __string(device_id, __dev_name_bo(bo))
> > >  		     __field(bool, move_lacks_source)
> > >  			),
> > > @@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
> > >  	    TP_fast_assign(
> > >  		   __entry->bo      = bo;
> > >  		   __entry->size = bo->size;
> > > -		   __entry->new_placement = new_placement;
> > > -		   __entry->old_placement = old_placement;
> > > +		   __assign_str(new_placement_name);
> > > +		   __assign_str(old_placement_name);
> > >  		   __assign_str(device_id);
> > >  		   __entry->move_lacks_source = move_lacks_source;
> > >  		   ),
> > >  	    TP_printk("move_lacks_source:%s, migrate object %p
> > > [size %zu] from %s to %s device_id:%s",
> > >  		      __entry->move_lacks_source ? "yes" : "no",
> > > __entry->bo, __entry->size,
> > > -		      xe_mem_type_to_name[__entry->old_placement],
> > > -		      xe_mem_type_to_name[__entry->new_placement],
> > > __get_str(device_id))
> > > +		      __get_str(old_placement_name),
> > > +		      __get_str(new_placement_name),
> > > __get_str(device_id))
> > >  );
> > >  
> > >  DECLARE_EVENT_CLASS(xe_vma,
> > > -- 
> > > 2.47.1
> > > 
> > > 
> 
> 

Re: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Thomas Hellström]

On Mon, 2024-12-23 at 17:04 +0000, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Thomas Hellström <thomas.hellstrom@linux.intel.com> 
> Sent: Monday, December 23, 2024 7:57 AM
> To: Cavitt, Jonathan <jonathan.cavitt@intel.com>;
> intel-xe@lists.freedesktop.org
> Cc: Sousa, Gustavo <gustavo.sousa@intel.com>; De Marchi, Lucas
> <lucas.demarchi@intel.com>; Radhakrishna Sripada
> <radhakrishna.sripada@intel.com>; Roper, Matthew D
> <matthew.d.roper@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF
> > 
> > On Mon, 2024-12-23 at 15:44 +0000, Cavitt, Jonathan wrote:
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf
> > > Of
> > > Thomas Hellström
> > > Sent: Monday, December 23, 2024 5:43 AM
> > > To: intel-xe@lists.freedesktop.org
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Sousa,
> > > Gustavo <gustavo.sousa@intel.com>; De Marchi, Lucas
> > > <lucas.demarchi@intel.com>; Radhakrishna Sripada
> > > <radhakrishna.sripada@intel.com>; Roper, Matthew D
> > > <matthew.d.roper@intel.com>; Vivi, Rodrigo
> > > <rodrigo.vivi@intel.com>;
> > > stable@vger.kernel.org
> > > Subject: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF
> > > > 
> > > > The commit
> > > > afd2627f727b ("tracing: Check "%s" dereference via the field
> > > > and
> > > > not the TP_printk format")
> > > > exposes potential UAFs in the xe_bo_move trace event.
> > > > 
> > > > Fix those by avoiding dereferencing the
> > > > xe_mem_type_to_name[] array at TP_printk time.
> > > > 
> > > > Since some code refactoring has taken place, explicit
> > > > backporting
> > > > may
> > > > be needed for kernels older than 6.10.
> > > > 
> > > > Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma
> > > > traces")
> > > > Cc: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > Cc: intel-xe@lists.freedesktop.org
> > > > Cc: <stable@vger.kernel.org> # v6.11+
> > > > Signed-off-by: Thomas Hellström
> > > > <thomas.hellstrom@linux.intel.com>
> > > 
> > > I take it we're hitting the WARN_ONCE in ignore_event due to a
> > > test_safe_str failure?
> > 
> > Actually it's the WARN_ONCE in test_event_printk()
> > 
> > if (WARN_ON_ONCE(dereference_flags)) {
> 
> Ah, I see.
> 
> There's a comment above that WARN_ON_ONCE as well, and it
> more or less recommends the same actions, albeit with less
> specificity.  My RB still stands.

Thanks. I'll push this commit with that R-B to get CI running.

/Thomas


> -Jonathan Cavitt
> 
> > 
> > 
> > > I don't know about us hitting a UAF here, but this fix is exactly
> > > what was recommended
> > > in the comment immediately above the WARN_ONCE that we shouldn't
> > > be
> > > hitting, so
> > > this is probably correct if that's what we're trying to avoid.
> > 
> > I'll double-check to see if I can easily trigger the UAF.
> > 
> > 
> > > Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> > 
> > Thanks,
> > Thomas
> > 
> > 
> > > -Jonathan Cavitt
> > > 
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h
> > > > b/drivers/gpu/drm/xe/xe_trace_bo.h
> > > > index 1762dd30ba6d..ea50fee50c7d 100644
> > > > --- a/drivers/gpu/drm/xe/xe_trace_bo.h
> > > > +++ b/drivers/gpu/drm/xe/xe_trace_bo.h
> > > > @@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
> > > >  	    TP_STRUCT__entry(
> > > >  		     __field(struct xe_bo *, bo)
> > > >  		     __field(size_t, size)
> > > > -		     __field(u32, new_placement)
> > > > -		     __field(u32, old_placement)
> > > > +		     __string(new_placement_name,
> > > > xe_mem_type_to_name[new_placement])
> > > > +		     __string(old_placement_name,
> > > > xe_mem_type_to_name[old_placement])
> > > >  		     __string(device_id, __dev_name_bo(bo))
> > > >  		     __field(bool, move_lacks_source)
> > > >  			),
> > > > @@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
> > > >  	    TP_fast_assign(
> > > >  		   __entry->bo      = bo;
> > > >  		   __entry->size = bo->size;
> > > > -		   __entry->new_placement = new_placement;
> > > > -		   __entry->old_placement = old_placement;
> > > > +		   __assign_str(new_placement_name);
> > > > +		   __assign_str(old_placement_name);
> > > >  		   __assign_str(device_id);
> > > >  		   __entry->move_lacks_source =
> > > > move_lacks_source;
> > > >  		   ),
> > > >  	    TP_printk("move_lacks_source:%s, migrate object %p
> > > > [size %zu] from %s to %s device_id:%s",
> > > >  		      __entry->move_lacks_source ? "yes" :
> > > > "no",
> > > > __entry->bo, __entry->size,
> > > > -		      xe_mem_type_to_name[__entry-
> > > > >old_placement],
> > > > -		      xe_mem_type_to_name[__entry-
> > > > >new_placement],
> > > > __get_str(device_id))
> > > > +		      __get_str(old_placement_name),
> > > > +		      __get_str(new_placement_name),
> > > > __get_str(device_id))
> > > >  );
> > > >  
> > > >  DECLARE_EVENT_CLASS(xe_vma,
> > > > -- 
> > > > 2.47.1
> > > > 
> > > > 
> > 
> > 

Re: [PATCH] drm/xe/tracing: Fix a potential TP_printk UAF

[Lucas De Marchi]

On Mon, Dec 23, 2024 at 05:33:10PM +0100, Thomas Hellström wrote:
>On Mon, 2024-12-23 at 07:44 -0800, Matthew Brost wrote:
>> On Mon, Dec 23, 2024 at 02:42:50PM +0100, Thomas Hellström wrote:
>> > The commit
>> > afd2627f727b ("tracing: Check "%s" dereference via the field and
>> > not the TP_printk format")
>> > exposes potential UAFs in the xe_bo_move trace event.
>> >
>> > Fix those by avoiding dereferencing the
>> > xe_mem_type_to_name[] array at TP_printk time.
>> >
>> > Since some code refactoring has taken place, explicit backporting
>> > may
>> > be needed for kernels older than 6.10.
>> >
>> > Fixes: e46d3f813abd ("drm/xe/trace: Extract bo, vm, vma traces")
>> > Cc: Gustavo Sousa <gustavo.sousa@intel.com>
>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
>> > Cc: Matt Roper <matthew.d.roper@intel.com>
>> > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > Cc: intel-xe@lists.freedesktop.org
>> > Cc: <stable@vger.kernel.org> # v6.11+
>> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/xe/xe_trace_bo.h | 12 ++++++------
>> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_trace_bo.h
>> > b/drivers/gpu/drm/xe/xe_trace_bo.h
>> > index 1762dd30ba6d..ea50fee50c7d 100644
>> > --- a/drivers/gpu/drm/xe/xe_trace_bo.h
>> > +++ b/drivers/gpu/drm/xe/xe_trace_bo.h
>> > @@ -60,8 +60,8 @@ TRACE_EVENT(xe_bo_move,
>> >  	    TP_STRUCT__entry(
>> >  		     __field(struct xe_bo *, bo)
>> >  		     __field(size_t, size)
>> > -		     __field(u32, new_placement)
>> > -		     __field(u32, old_placement)
>> > +		     __string(new_placement_name,
>> > xe_mem_type_to_name[new_placement])
>> > +		     __string(old_placement_name,
>> > xe_mem_type_to_name[old_placement])
>> >  		     __string(device_id, __dev_name_bo(bo))
>> >  		     __field(bool, move_lacks_source)
>> >  			),
>> > @@ -69,15 +69,15 @@ TRACE_EVENT(xe_bo_move,
>> >  	    TP_fast_assign(
>> >  		   __entry->bo      = bo;
>> >  		   __entry->size = bo->size;
>> > -		   __entry->new_placement = new_placement;
>> > -		   __entry->old_placement = old_placement;
>> > +		   __assign_str(new_placement_name);
>> > +		   __assign_str(old_placement_name);
>> >  		   __assign_str(device_id);
>> >  		   __entry->move_lacks_source = move_lacks_source;
>> >  		   ),
>> >  	    TP_printk("move_lacks_source:%s, migrate object %p
>> > [size %zu] from %s to %s device_id:%s",
>> >  		      __entry->move_lacks_source ? "yes" : "no",
>> > __entry->bo, __entry->size,
>> > -		      xe_mem_type_to_name[__entry->old_placement],
>> > -		      xe_mem_type_to_name[__entry->new_placement],
>> > __get_str(device_id))
>>
>> So is this the UAF? i.e., The Xe module unloads and
>> xe_mem_type_to_name
>> is gone?
>
>I would imagine that's the intention of the warning. However removing
>the xe_module seems to empty the trace buffer of xe_bo_move events.
>Whether there is a race in that process or whether the TP_printk check
>can't distinguish between module local addresses and other addresses is
>hard to tell. In any case, it looks like we need to comply with the
>warning here (I suspect CI refuses to run otherwise), and since it only
>appears to trigger on the xe module load on my system, it's unlikely
>that mentioned commit will be reverted.

it's actually a false positive from afd2627f727b. See explanation
by Steve at https://lore.kernel.org/all/20241230141329.5f698715@batman.local.home/
the fix later on

Lucas De Marchi


>
>
>>
>> I noticed that xe_mem_type_to_name is not static, it likely should
>> be.
>> Would that help here?
>
>No, doesn't help unfortunately.
>
>/Thomas
>
>
>
>>
>> Matt
>>
>> > +		      __get_str(old_placement_name),
>> > +		      __get_str(new_placement_name),
>> > __get_str(device_id))
>> >  );
>> >  
>> >  DECLARE_EVENT_CLASS(xe_vma,
>> > --
>> > 2.47.1
>> >
>