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

[Lionel Landwerlin]

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.

Cheers,

-
Lionel

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