[Mesa-dev] [PATCH v3 20/48] intel/fs: Protect opt_algebraic from OOB BROADCAST indices

[Iago Toral]

On Fri, 2017-10-27 at 12:28 -0700, Jason Ekstrand wrote:
> On Fri, Oct 27, 2017 at 12:09 AM, Iago Toral <itoral at igalia.com>
> wrote:
> > On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> > 
> > > ---
> > 
> > >  src/intel/compiler/brw_fs.cpp | 10 ++++++++--
> > 
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > >
> > 
> > > diff --git a/src/intel/compiler/brw_fs.cpp
> > 
> > > b/src/intel/compiler/brw_fs.cpp
> > 
> > > index 1c4351b..52079d3 100644
> > 
> > > --- a/src/intel/compiler/brw_fs.cpp
> > 
> > > +++ b/src/intel/compiler/brw_fs.cpp
> > 
> > > @@ -2416,8 +2416,14 @@ fs_visitor::opt_algebraic()
> > 
> > >              progress = true;
> > 
> > >           } else if (inst->src[1].file == IMM) {
> > 
> > >              inst->opcode = BRW_OPCODE_MOV;
> > 
> > > -            inst->src[0] = component(inst->src[0],
> > 
> > > -                                     inst->src[1].ud);
> > 
> > > +            /* It's possible that the selected component will be
> > too
> > 
> > > large and
> > 
> > > +             * overflow the register.  If this happens and we
> > some
> > 
> > > how manage
> > 
> > > +             * to constant fold it in and get here, it would
> > cause
> > 
> > > an assert
> > 
> > > +             * in component() below.  Instead, just let it wrap
> > 
> > > around if it
> > 
> > > +             * goes over exec_size.
> > 
> > > +             */
> > 
> > 
> > 
> > component() is really a horiz_offset() call which is in turn a
> > 
> > byte_offset() call, which doesn't assert on anything other than
> > invalid
> > 
> > register files. I guess you mean that the byte offset computed by
> > the
> > 
> > component() call below can later lead to hitting assertions as we
> > 
> > attempt to read outside the allocated space for the vgrf, right?
> 
> Yes.  Nothing asserts here, we just end up reading way past the end
> of the VGRF and the validator which checks whether or not we keep all
> our reads in-bounds throws a fit.
>  
> > My question is whether this is supposed to happen at all, it seems
> > like
> > 
> > we should not be emitting broadcast operations like this at all
> > since
> > 
> > they are invalid and here we are instead papering over that bug.
> > 
> > 
> 
> This can happen if someone does a read_invocation from SPIR-V or GLSL
> that has an invocation index that is too large.  We're allowed to
> give them garbage values, but asserting isn't nice.

Oh, the bogus value can be directly provided by the user... I don't
know if asserting in this case is more or less useful to the user to be
honest, I guess the assert message won't be very useful to users
anyway.

> I've updated the comment to:
> 
>             /* It's possible that the selected component will be too
> large and
>              * overflow the register.  This can happen if someone
> does a
>              * readInvocation() from GLSL or SPIR-V and provides an
> OOB
>              * invocationIndex.  If this happens and we some how
> manage
>              * to constant fold it in and get here, then component()
> may cause
>              * us to start reading outside of the VGRF which will
> lead to an
>              * assert later.  Instead, just let it wrap around if it
> goes over
>              * exec_size.
>              */

Thanks! I think this makes the scenario more clear for future
reference.

Iago
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171030/b8238996/attachment.html>