[v2] intel/decoder: fix the possible out of bounds group_iter

Submitted by andrey simiklit on Aug. 14, 2018, 9:04 a.m.

Details

Message ID 1534237461-15199-1-git-send-email-asimiklit.work@gmail.com
State New
Headers show
Series "intel/decoder: fix the possible out of bounds group_iter" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Aug. 14, 2018, 9:04 a.m.
From: Andrii Simiklit <asimiklit.work@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@globallogic.com>
---
 src/intel/common/gen_decoder.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

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;

Comments

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@gmail.com wrote:
> From: Andrii Simiklit <asimiklit.work@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@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;
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@gmail.com wrote:
>> From: Andrii Simiklit <asimiklit.work@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@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;
>
>
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@gmail.com wrote:
>>> From: Andrii Simiklit <asimiklit.work@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
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?

> 
> > 
> > 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@gmail.com wrote:
> > > > From: Andrii Simiklit <asimiklit.work@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@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
>
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@gmail.com wrote:
>>>>> From: Andrii Simiklit <asimiklit.work@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@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@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>