[igt-dev] [PATCH i-g-t] lib/kms: Range check pipe before lookup

[Petri Latvala]

On Thu, Nov 19, 2020 at 09:00:32AM +0000, Chris Wilson wrote:
> Quoting Petri Latvala (2020-11-19 08:53:04)
> > On Thu, Nov 19, 2020 at 08:35:59AM +0000, Chris Wilson wrote:
> > > Make sure the lookup index is within the range of the table before
> > > accessing.
> > > 
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > ---
> > >  lib/igt_kms.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> > > index e5d8e82c9..9175e50fe 100644
> > > --- a/lib/igt_kms.c
> > > +++ b/lib/igt_kms.c
> > > @@ -1902,7 +1902,7 @@ static void igt_fill_display_format_mod(igt_display_t *display);
> > >   */
> > >  void igt_require_pipe(igt_display_t *display, enum pipe pipe)
> > >  {
> > > -     igt_skip_on_f(!display->pipes[pipe].enabled,
> > > +     igt_skip_on_f(pipe >= display->n_pipes || !display->pipes[pipe].enabled,
> > >                       "Pipe %s does not exist or not enabled\n",
> > >                       kmstest_pipe_name(pipe));
> > 
> > Who's calling this with an invalid 'pipe' value? display->pipes size
> > is no longer display->n_pipes, right?
> 
> https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9356/shard-skl8/igt@kms_pipe_crc_basic@disable-crc-after-crtc-pipe-b.html
> Starting subtest: disable-crc-after-crtc-pipe-B
> Received signal SIGSEGV.
> Stack trace:
>  #0 [fatal_sig_handler+0xd6]
>  #1 [killpg+0x40]
>  #2 [igt_output_get_mode+0x0]
>  #3 [__real_main261+0x629]
>  #4 [main+0x27]
>  #5 [__libc_start_main+0xe7]
>  #6 [_start+0x2a]
> Subtest disable-crc-after-crtc-pipe-B: CRASH (0.087s)
> 
> Suggests that the output is NULL/garbage, so I was looking for ideas as
> to how we end up dying there. The loop in test_read_crc() doesn't look
> susceptible to corrupting the local variable, so I guess the output was
> suspect to begin with.


Hmm yeah.

Other tests are explicitly checking that
igt_get_single_output_for_pipe returns non-null, that one doesn't...

In fact it doesn't even call igt_require_pipe. Even before the support
for fused pipes was merged, that test looks broken.


-- 
Petri Latvala