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

[Lionel Landwerlin]

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