diff mbox series

[1/4] drm/i915/display: Fix out-of-bounds access in pipe-related tracepoints

Message ID 20240829220106.80449-2-gustavo.sousa@intel.com (mailing list archive)
State New, archived
Headers show
Series Miscelaneous fixes for display tracepoints | expand

Commit Message

Gustavo Sousa Aug. 29, 2024, 10 p.m. UTC 
Some display trace events use array members to store frame and scanline
counts for each pipe. However, those arrays are declared with 3 as the
hardcoded size, which cause out-of-bounds access when the trace event is
enabled on a platform that contains pipe D.

For example, when looking at the last 10 intel_pipe_enable events after
running IGT's testdisplay, we see the following on a MTL machine that
has pipe D available:

    $ trace-cmd report -R -F intel_pipe_enable \
    > | tail \
    > | sed 's,\(frame=.*\) \(scanline=.*\),\n\t   \1\n\t\2,'
         testdisplay-6715  [002] 17591.063491: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[83, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [003] 17591.264742: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[89, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [003] 17591.464541: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[8f, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [001] 17591.695827: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[95, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [000] 17591.915841: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[9a, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [000] 17592.127114: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[a0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [002] 17592.358351: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[a8, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [002] 17592.580467: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[ae, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [000] 17592.950946: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[b8, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-6715  [004] 17593.079597: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[bf, 01, 00, 00, 01, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[00, 00, 00, 00, 3a, 04, 00, 00, 00, 00, 00, 00] pipe=1

Which shows zeros for pipe A's scanline counts. That happens because
pipe D's frame counts are overwriting them.

Let's fix that by making the arrays bring able to store info for all
possible pipes.

With the fix, we get the following:

    $ trace-cmd report -R -F intel_pipe_enable \
    > | tail \
    > | sed 's,\(frame=.*\) \(scanline=.*\),\n\t   \1\n\t\2,'
         testdisplay-7040  [003] 18067.489565: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[8c, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[8e, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [002] 18067.699312: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[92, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[58, 02, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [002] 18067.908868: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[98, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[58, 02, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [002] 18068.122802: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[9d, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[58, 02, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [003] 18068.331019: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[a2, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[e0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [002] 18068.529067: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[a8, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[e0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [003] 18068.742033: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[ae, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[e0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [002] 18068.956229: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[b3, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[1f, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [002] 18069.295322: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[bb, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[7b, 08, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
         testdisplay-7040  [010] 18069.423527: intel_pipe_enable:     dev=0000:00:02.0
               frame=ARRAY[c2, 01, 00, 00, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
            scanline=ARRAY[d0, 05, 00, 00, 3a, 04, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=1

Which makes more sense now.

Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display_trace.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Matt Roper Sept. 18, 2024, 10:13 p.m. UTC | #1 
On Thu, Aug 29, 2024 at 07:00:44PM -0300, Gustavo Sousa wrote:
> Some display trace events use array members to store frame and scanline
> counts for each pipe. However, those arrays are declared with 3 as the
> hardcoded size, which cause out-of-bounds access when the trace event is
> enabled on a platform that contains pipe D.
> 
> For example, when looking at the last 10 intel_pipe_enable events after
> running IGT's testdisplay, we see the following on a MTL machine that
> has pipe D available:
> 
>     $ trace-cmd report -R -F intel_pipe_enable \
>     > | tail \
>     > | sed 's,\(frame=.*\) \(scanline=.*\),\n\t   \1\n\t\2,'
>          testdisplay-6715  [002] 17591.063491: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[83, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [003] 17591.264742: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[89, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [003] 17591.464541: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[8f, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [001] 17591.695827: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[95, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [000] 17591.915841: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[9a, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [000] 17592.127114: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[a0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [002] 17592.358351: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[a8, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [002] 17592.580467: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[ae, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [000] 17592.950946: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[b8, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-6715  [004] 17593.079597: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[bf, 01, 00, 00, 01, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[00, 00, 00, 00, 3a, 04, 00, 00, 00, 00, 00, 00] pipe=1
> 
> Which shows zeros for pipe A's scanline counts. That happens because
> pipe D's frame counts are overwriting them.
> 
> Let's fix that by making the arrays bring able to store info for all
> possible pipes.
> 
> With the fix, we get the following:
> 
>     $ trace-cmd report -R -F intel_pipe_enable \
>     > | tail \
>     > | sed 's,\(frame=.*\) \(scanline=.*\),\n\t   \1\n\t\2,'
>          testdisplay-7040  [003] 18067.489565: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[8c, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[8e, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [002] 18067.699312: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[92, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[58, 02, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [002] 18067.908868: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[98, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[58, 02, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [002] 18068.122802: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[9d, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[58, 02, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [003] 18068.331019: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[a2, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[e0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [002] 18068.529067: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[a8, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[e0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [003] 18068.742033: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[ae, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[e0, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [002] 18068.956229: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[b3, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[1f, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [002] 18069.295322: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[bb, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[7b, 08, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=0
>          testdisplay-7040  [010] 18069.423527: intel_pipe_enable:     dev=0000:00:02.0
>                frame=ARRAY[c2, 01, 00, 00, 01, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00]
>             scanline=ARRAY[d0, 05, 00, 00, 3a, 04, 00, 00, 00, 00, 00, 00, 00, 00, 00, 00] pipe=1
> 
> Which makes more sense now.
> 
> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I guess nobody has really needed to use these tracepoints much for
debugging since TGL added the 4th pipe.


Matt

> ---
>  drivers/gpu/drm/i915/display/intel_display_trace.h | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_trace.h b/drivers/gpu/drm/i915/display/intel_display_trace.h
> index c734ef1fba3c..8a3185862089 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_trace.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_trace.h
> @@ -15,6 +15,7 @@
>  
>  #include "i915_drv.h"
>  #include "intel_crtc.h"
> +#include "intel_display_limits.h"
>  #include "intel_display_types.h"
>  #include "intel_vblank.h"
>  
> @@ -27,8 +28,8 @@ TRACE_EVENT(intel_pipe_enable,
>  
>  	    TP_STRUCT__entry(
>  			     __string(dev, __dev_name_kms(crtc))
> -			     __array(u32, frame, 3)
> -			     __array(u32, scanline, 3)
> +			     __array(u32, frame, I915_MAX_PIPES)
> +			     __array(u32, scanline, I915_MAX_PIPES)
>  			     __field(enum pipe, pipe)
>  			     ),
>  	    TP_fast_assign(
> @@ -55,8 +56,8 @@ TRACE_EVENT(intel_pipe_disable,
>  
>  	    TP_STRUCT__entry(
>  			     __string(dev, __dev_name_kms(crtc))
> -			     __array(u32, frame, 3)
> -			     __array(u32, scanline, 3)
> +			     __array(u32, frame, I915_MAX_PIPES)
> +			     __array(u32, scanline, I915_MAX_PIPES)
>  			     __field(enum pipe, pipe)
>  			     ),
>  
> @@ -184,8 +185,8 @@ TRACE_EVENT(intel_memory_cxsr,
>  
>  	    TP_STRUCT__entry(
>  			     __string(dev, __dev_name_i915(dev_priv))
> -			     __array(u32, frame, 3)
> -			     __array(u32, scanline, 3)
> +			     __array(u32, frame, I915_MAX_PIPES)
> +			     __array(u32, scanline, I915_MAX_PIPES)
>  			     __field(bool, old)
>  			     __field(bool, new)
>  			     ),
> -- 
> 2.46.0
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display_trace.h b/drivers/gpu/drm/i915/display/intel_display_trace.h
index c734ef1fba3c..8a3185862089 100644
--- a/drivers/gpu/drm/i915/display/intel_display_trace.h
+++ b/drivers/gpu/drm/i915/display/intel_display_trace.h
@@ -15,6 +15,7 @@ 
 
 #include "i915_drv.h"
 #include "intel_crtc.h"
+#include "intel_display_limits.h"
 #include "intel_display_types.h"
 #include "intel_vblank.h"
 
@@ -27,8 +28,8 @@  TRACE_EVENT(intel_pipe_enable,
 
 	    TP_STRUCT__entry(
 			     __string(dev, __dev_name_kms(crtc))
-			     __array(u32, frame, 3)
-			     __array(u32, scanline, 3)
+			     __array(u32, frame, I915_MAX_PIPES)
+			     __array(u32, scanline, I915_MAX_PIPES)
 			     __field(enum pipe, pipe)
 			     ),
 	    TP_fast_assign(
@@ -55,8 +56,8 @@  TRACE_EVENT(intel_pipe_disable,
 
 	    TP_STRUCT__entry(
 			     __string(dev, __dev_name_kms(crtc))
-			     __array(u32, frame, 3)
-			     __array(u32, scanline, 3)
+			     __array(u32, frame, I915_MAX_PIPES)
+			     __array(u32, scanline, I915_MAX_PIPES)
 			     __field(enum pipe, pipe)
 			     ),
 
@@ -184,8 +185,8 @@  TRACE_EVENT(intel_memory_cxsr,
 
 	    TP_STRUCT__entry(
 			     __string(dev, __dev_name_i915(dev_priv))
-			     __array(u32, frame, 3)
-			     __array(u32, scanline, 3)
+			     __array(u32, frame, I915_MAX_PIPES)
+			     __array(u32, scanline, I915_MAX_PIPES)
 			     __field(bool, old)
 			     __field(bool, new)
 			     ),