[Mesa-dev] [PATCH v2] intel/decoder: fix the possible out of bounds group_iter
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Mon Aug 20 09:46:19 UTC 2018
On 15/08/18 10:45, andrey simiklit wrote:
> 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.
Sorry for the late answer :(
This implies an unknown size for the inspected instruction/struct.
I think this shouldn't happen because the caller even try to initialize
the iterator to decode it.
I would add an assert, because the iterator doesn't really deal with
that case.
I'm not sure whether there is such case in the genxml files, but if it
happens we should probably look into it.
Cheers,
-
Lionel
>
> Regards,
> Andrii.
>
> On Tue, Aug 14, 2018 at 7:08 PM, Lionel Landwerlin
> <lionel.g.landwerlin at intel.com <mailto: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 <http://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
> <mailto:asimiklit.work at gmail.com> wrote:
>
> From: Andrii Simiklit
> <asimiklit.work at gmail.com
> <mailto: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
> <https://bugs.freedesktop.org/show_bug.cgi?id=107544>
> Signed-off-by: Andrii Simiklit
> <andrii.simiklit at globallogic.com
> <mailto: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
> <mailto:mesa-dev at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180820/8bc230f0/attachment-0001.html>
More information about the mesa-dev
mailing list