<div dir="ltr"><div>Hi all,</div><div><br></div><div>Thanks for your reply.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>We shouldn't even get to use the iterator if it's an unknown instruction.</div>
The decoder should just advance dword by dword until it finds something that<br>
makes sense again.<br></blockquote><div><br></div><div>Got it) <br></div><div>So this is an expected behavior there:</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>return iter_group_offset_bits(iter, iter->group_iter + 1) <<br> (gen_group_get_length(iter->group, iter->p) * 32);<br></div></blockquote><div><br></div><div>when we convert a negative <b>int</b> to <b>uint</b> to return true to continue our loop.<br></div><div><br></div><div><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>return iter_group_offset_bits(iter, iter->group_iter + 1) <<br> (<b>0xFFFFFFE0U</b>);</div></blockquote><div><br></div><div>Do you think it is good idea to add comment or something like this into the "iter_more_groups" function:<br><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>int <b>length</b> = gen_group_get_length(iter->gro<wbr>up, iter->p); </div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">return <b>length < 0 ||</b><br> iter_group_offset_bits(iter, iter->group_iter + 1) < <br> (<b>length</b> * 32);<br></blockquote><div> </div>to show more explicitly here that we want to return true to continue our loop<br>when the -1 is returned from the "gen_group_get_length" function <br></div><div>because at the moment it is a bit implicit)</div></div><div>Please let me know if I am incorrect.</div><div><br></div><div>Regards,</div><div>Andrii.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 14, 2018 at 7:08 PM, Lionel Landwerlin <span dir="ltr"><<a href="mailto:lionel.g.landwerlin@intel.com" target="_blank">lionel.g.landwerlin@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 14/08/18 16:16, Rafael Antognolli wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On Tue, Aug 14, 2018 at 03:36:18PM +0100, Lionel Landwerlin wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On 14/08/18 12:55, <a href="http://asimiklit.work" rel="noreferrer" target="_blank">asimiklit.work</a> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi Lionel,<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi Andrii,<br>
<br>
Again sorry, I don't think this is the right fix.<br>
I'm sending another patch to fix the parsing of<br>
MI_BATCH_BUFFER_START which seems to be the actual issue.<br>
<br>
Thanks for working on this,<br>
</blockquote>
Thanks for your fast reply.<br>
I agree that it is not correct patch for this issue but anyway<br>
"iter_more_groups" function will still work incorrectly<br>
for unknown instructions when the "iter->group->variable" field is true.<br>
I guess that this case should be fixed.<br>
Please let me know if I am incorrect.<br>
</blockquote>
Hey Andrii,<br>
<br>
We shouldn't even get to use the iterator if it's an unknown instruction.<br>
The decoder should just advance dword by dword until it finds something that<br>
makes sense again.<br>
<br>
If we run into that problem, I think we should fix the caller.<br>
</blockquote>
In that case, would an unreachable() or assert be a good thing to do?<br>
</blockquote>
<br>
Yep, I guess assert in gen_field_iterator_init() should be a good thing.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Regards,<br>
Andrii.<br>
<br>
On 2018-08-14 1:26 PM, Lionel Landwerlin wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Hi Andrii,<br>
<br>
Again sorry, I don't think this is the right fix.<br>
I'm sending another patch to fix the parsing of<br>
MI_BATCH_BUFFER_START which seems to be the actual issue.<br>
<br>
Thanks for working on this,<br>
<br>
-<br>
Lionel<br>
<br>
On 14/08/18 10:04, <a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
From: Andrii Simiklit <<a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a>><br>
<br>
The "gen_group_get_length" function can return a negative value<br>
and it can lead to the out of bounds group_iter.<br>
<br>
v2: printing of "unknown command type" was added<br>
Bugzilla: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=107544" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/s<wbr>how_bug.cgi?id=107544</a><br>
Signed-off-by: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.c<wbr>om</a>><br>
---<br>
src/intel/common/gen_decoder.c | 13 +++++++++++--<br>
1 file changed, 11 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/src/intel/common/gen_decoder<wbr>.c<br>
b/src/intel/common/gen_decoder<wbr>.c<br>
index ec0a486..b36facf 100644<br>
--- a/src/intel/common/gen_decoder<wbr>.c<br>
+++ b/src/intel/common/gen_decoder<wbr>.c<br>
@@ -770,6 +770,13 @@ gen_group_get_length(struct gen_group<br>
*group, const uint32_t *p)<br>
return -1;<br>
}<br>
}<br>
+ default: {<br>
+ fprintf(stderr, "Unknown command type %u in '%s::%s'\n",<br>
+ type,<br>
+ (group->parent && group->parent->name) ?<br>
group->parent->name : "UNKNOWN",<br>
+ group->name ? group->name : "UNKNOWN");<br>
+ break;<br>
+ }<br>
}<br>
return -1;<br>
@@ -803,8 +810,10 @@ static bool<br>
iter_more_groups(const struct gen_field_iterator *iter)<br>
{<br>
if (iter->group->variable) {<br>
- return iter_group_offset_bits(iter, iter->group_iter + 1) <<br>
- (gen_group_get_length(iter->gr<wbr>oup, iter->p) * 32);<br>
+ const int length = gen_group_get_length(iter->gro<wbr>up, iter->p);<br>
+ return length > 0 &&<br>
+ iter_group_offset_bits(iter, iter->group_iter + 1) <<br>
+ (length * 32);<br>
} else {<br>
return (iter->group_iter + 1) < iter->group->group_count ||<br>
iter->group->next != NULL;<br>
</blockquote>
<br>
</blockquote>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</blockquote>
<br>
</blockquote></blockquote>
<br>
</blockquote></div><br></div></div>