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

[andrey simiklit]

Hi all,

Thanks for your reply.

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

Got it)
So this is an expected behavior there:

return iter_group_offset_bits(iter, iter->group_iter + 1) <
>               (gen_group_get_length(iter->group, iter->p) * 32);
>

when we convert a negative *int* to *uint* to return true to continue our
loop.

return iter_group_offset_bits(iter, iter->group_iter + 1) <
>               (*0xFFFFFFE0U*);
>

Do you think it is good idea to add comment or something like this into the
"iter_more_groups" function:

int *length* = gen_group_get_length(iter->group, iter->p);
>
return *length < 0 ||*
>            iter_group_offset_bits(iter, iter->group_iter + 1) <
>             (*length* * 32);
>

to show more explicitly here that we want to return true to continue our
loop
when the -1 is returned from the "gen_group_get_length" function
because at the moment it is a bit implicit)
Please let me know if I am incorrect.

Regards,
Andrii.

On Tue, Aug 14, 2018 at 7:08 PM, Lionel Landwerlin <
lionel.g.landwerlin at intel.com> wrote:

> On 14/08/18 16:16, Rafael Antognolli wrote:
>
>> 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?
>>
>
> Yep, I guess assert in gen_field_iterator_init() should be a good thing.
>
>
>> 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
>>>>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180815/d6396965/attachment.html>