[Mesa-dev] [PATCH v2] intel/decoder: fix the possible out of bounds group_iter

[Rafael Antognolli]

On Tue, Aug 14, 2018 at 03:36:18PM +0100, Lionel Landwerlin wrote:
> On 14/08/18 12:55, asimiklit.work wrote:
> > Hi Lionel,
> > > Hi Andrii,
> > > 
> > > Again sorry, I don't think this is the right fix.
> > > I'm sending another patch to fix the parsing of
> > > MI_BATCH_BUFFER_START which seems to be the actual issue.
> > > 
> > > Thanks for working on this,
> > Thanks for your fast reply.
> > I agree that it is not correct patch for this issue but anyway
> > "iter_more_groups" function will still work incorrectly
> > for unknown instructions when the "iter->group->variable" field is true.
> > I guess that this case should be fixed.
> > Please let me know if I am incorrect.
> 
> Hey Andrii,
> 
> We shouldn't even get to use the iterator if it's an unknown instruction.
> The decoder should just advance dword by dword until it finds something that
> makes sense again.
> 
> If we run into that problem, I think we should fix the caller.

In that case, would an unreachable() or assert be a good thing to do?

> 
> > 
> > Regards,
> > Andrii.
> > 
> > On 2018-08-14 1:26 PM, Lionel Landwerlin wrote:
> > > Hi Andrii,
> > > 
> > > Again sorry, I don't think this is the right fix.
> > > I'm sending another patch to fix the parsing of
> > > MI_BATCH_BUFFER_START which seems to be the actual issue.
> > > 
> > > Thanks for working on this,
> > > 
> > > -
> > > Lionel
> > > 
> > > On 14/08/18 10:04, asimiklit.work at gmail.com wrote:
> > > > From: Andrii Simiklit <asimiklit.work at gmail.com>
> > > > 
> > > > The "gen_group_get_length" function can return a negative value
> > > > and it can lead to the out of bounds group_iter.
> > > > 
> > > > v2: printing of "unknown command type" was added
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107544
> > > > Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
> > > > ---
> > > >   src/intel/common/gen_decoder.c | 13 +++++++++++--
> > > >   1 file changed, 11 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/intel/common/gen_decoder.c
> > > > b/src/intel/common/gen_decoder.c
> > > > index ec0a486..b36facf 100644
> > > > --- a/src/intel/common/gen_decoder.c
> > > > +++ b/src/intel/common/gen_decoder.c
> > > > @@ -770,6 +770,13 @@ gen_group_get_length(struct gen_group
> > > > *group, const uint32_t *p)
> > > >               return -1;
> > > >         }
> > > >      }
> > > > +   default: {
> > > > +      fprintf(stderr, "Unknown command type %u in '%s::%s'\n",
> > > > +            type,
> > > > +            (group->parent && group->parent->name) ?
> > > > group->parent->name : "UNKNOWN",
> > > > +            group->name ? group->name : "UNKNOWN");
> > > > +      break;
> > > > +   }
> > > >      }
> > > >        return -1;
> > > > @@ -803,8 +810,10 @@ static bool
> > > >   iter_more_groups(const struct gen_field_iterator *iter)
> > > >   {
> > > >      if (iter->group->variable) {
> > > > -      return iter_group_offset_bits(iter, iter->group_iter + 1) <
> > > > -              (gen_group_get_length(iter->group, iter->p) * 32);
> > > > +      const int length = gen_group_get_length(iter->group, iter->p);
> > > > +      return length > 0 &&
> > > > +            iter_group_offset_bits(iter, iter->group_iter + 1) <
> > > > +              (length * 32);
> > > >      } else {
> > > >         return (iter->group_iter + 1) < iter->group->group_count ||
> > > >            iter->group->next != NULL;
> > > 
> > > 
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>