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

[Rafael Antognolli]

On Fri, Aug 10, 2018 at 05:37:12PM +0100, Lionel Landwerlin wrote:
> Andrey also opened a bug about this issue :
> https://bugs.freedesktop.org/show_bug.cgi?id=107544
> 
> It feels like it should be fixed on master though. get_length() shouldn't
> return -1 for structs anymore.
> We should probably return 1 at end of get_length() so that the decoder
> prints out "unknown instruction".
> That would help spot potential errors and updates needed to genxml.

Yeah, that makes sense. I saw the we were doing the check for length < 0
somewhere else so it seemed reasonable to check for that, considering we
can return -1, but I agree that printing "unknown instruction" would be
better.

> -
> Lionel
> 
> 
> On 10/08/18 16:48, Rafael Antognolli wrote:
> > On Thu, Aug 09, 2018 at 03:00:30PM +0300, andrey simiklit wrote:
> > > Hi,
> > > 
> > > Sorry I missed the main thought here.
> > > The "gen_group_get_length" function returns int
> > > but the "iter_group_offset_bits" function returns uint32_t
> > > So uint32_t(int(-32)) = 0xFFFFFFE0U and it looks like unexpected behavior for
> > > me:
> > > iter_group_offset_bits(iter, iter->group_iter + 1) < 0xFFFFFFE0U;
> > That's fine, I think the original commit message is good enough to
> > understand this change. Feel free to add this extra bit too if you want,
> > but I don't think it's needed.
> > 
> > Reviewed-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > 
> > > Regards,
> > > Andrii.
> > > 
> > > On Thu, Aug 9, 2018 at 2:35 PM, Andrii Simiklit <asimiklit.work at gmail.com>
> > > wrote:
> > > 
> > >      The "gen_group_get_length" function can return a negative value
> > >      and it can lead to the out of bounds group_iter.
> > > 
> > >      Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
> > >      ---
> > >       src/intel/common/gen_decoder.c | 6 ++++--
> > >       1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > >      diff --git a/src/intel/common/gen_decoder.c b/src/intel/common/gen_decoder
> > >      .c
> > >      index ec0a486..f09bd87 100644
> > >      --- a/src/intel/common/gen_decoder.c
> > >      +++ b/src/intel/common/gen_decoder.c
> > >      @@ -803,8 +803,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;
> > >      --
> > >      2.7.4
> > > 
> > > 
> > > 
>