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

[asimiklit.work]

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.

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;
>
>