[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 at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: "Thomas Hellström" <thomas.hellstrom at linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: intel-xe at lists.freedesktop.org
> > Cc: <stable at vger.kernel.org> # v6.11+
> > Signed-off-by: Thomas Hellström <thomas.hellstrom at 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
> >