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

[asimiklit]

Hi all,

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

1. I am not sure about 'return 1' at end of the 'get_length'
because it will be some kind of the lie for all callers of this function.
As far as i see this function should notify caller (using -1 in this case)
that the length can not be determinated due to some reason and
this gives the ability to the caller to decide what to do in this case.
The 'return 1' here will deprive us of this ability.

Please share any thoughts about it.

2. I also think that the "unknown command" message in the unknown command case is very good idea.
I want to add the 'default' case to the switch at the end of 'get_length' function:

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

Because in my case even if I return 1 at end of the 'get_length' function
I will not receive "unknown instruction" message
which you already added in 'gen_print_batch' function on line 808
because I came to this function another way.

What do you think about it?

I will send the updated patch as soon as we come to agreement.

Regards,
Andrii.

On 10.08.18 20:32, Rafael Antognolli wrote:

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