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

[Jason Ekstrand]

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.

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.
             */

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171027/fd046a6b/attachment.html>