intel/decoder: fix the possible out of bounds group_iter

Submitted by andrey simiklit on Aug. 9, 2018, 11:35 a.m.

Details

Message ID 1533814525-8097-1-git-send-email-andrii.simiklit@globallogic.com
State New
Headers show
Series "intel/decoder: fix the possible out of bounds group_iter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Aug. 9, 2018, 11:35 a.m.
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@globallogic.com>
---
 src/intel/common/gen_decoder.c | 6 ++++--
 1 file changed, 4 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..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;

Comments

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

Regards,
Andrii.

On Thu, Aug 9, 2018 at 2:35 PM, Andrii Simiklit <asimiklit.work@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@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
>
>
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@intel.com>

> Regards,
> Andrii.
> 
> On Thu, Aug 9, 2018 at 2:35 PM, Andrii Simiklit <asimiklit.work@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@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
> 
> 
>
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.

-
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@intel.com>
>
>> Regards,
>> Andrii.
>>
>> On Thu, Aug 9, 2018 at 2:35 PM, Andrii Simiklit <asimiklit.work@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@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
>>
>>
>>
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@intel.com>
> > 
> > > Regards,
> > > Andrii.
> > > 
> > > On Thu, Aug 9, 2018 at 2:35 PM, Andrii Simiklit <asimiklit.work@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@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
> > > 
> > > 
> > > 
>
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@intel.com>
>>>
>>>> Regards,
>>>> Andrii.
>>>>
>>>> On Thu, Aug 9, 2018 at 2:35 PM, Andrii Simiklit <asimiklit.work@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@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
>>>>
>>>>
>>>>