Fix out-of-bounds read in shuftiDoubleExecReal tail handling#381
Fix out-of-bounds read in shuftiDoubleExecReal tail handling#381markos merged 1 commit intoVectorCamp:developfrom
Conversation
|
hi, this seems to be a duplicate of #368. I will release 5.4.13 by the end of this month. However, adding the extra unit test is not a bad idea. For small pieces of code, AI assistance is not a problem, if it was a whole component that would be a different issue and I would probably not accept it. |
|
I'm not sure #368 fixes the bug. I cherry-picked this commit on top of that PR ( |
|
ok, thanks for the extra test. I will check it myself this week. |
|
Great, thanks! |
|
I've not reviewed #368 yet, but I did review this one and it looks good to me. Thanks for catching that mistake. |
|
It is not a duplicate of #368. |
|
@Jongy Could you please rebase against current develop so that we get better idea of which tests might fail that are relevant ? Thanks. |
Jongy
force-pushed
the
fix-shufti-double-overread
branch
from
24513e8 to
6c9f2ef
Compare
Done :) |
markos
left a comment
markos
left a comment
There was a problem hiding this comment.
Thank you for your contribution to Vectorscan!
|
Actually I see that there are some cppcheck warnings, eg https://buildbot-ci.vectorcamp.gr/#/builders/274/builds/306 could you please fix those as well? |
Commit 9e9a10a ("Fix double shufti's vector end false positive", VectorCamp#325) changed the tail code in shuftiDoubleExecReal to read a full vector from the current pointer (loadu(d)), which can overread past buf_end by up to S-1 bytes. When the buffer ends at a page boundary followed by unmapped memory, this causes a SIGSEGV. Fix by reading the last S bytes backward from buf_end (matching the approach used in shuftiExecReal), and falling back to memcpy for buffers shorter than one vector width. Signed-off-by: Yonatan Goldschmidt <yonatan.goldschmidt@wiz.io>
Jongy
force-pushed
the
fix-shufti-double-overread
branch
from
6c9f2ef to
c041104
Compare
|
@markos I think I have, I'll monitor CI to see it's all green now. |
|
The remaining errors seem unrelated afaict (I also see the same errors in #383) |
yes ignore those, I'll deal with them separately. |
|
Great! We'll be waiting for a release with this fix |
|
@Jongy I'll create a release with most/all the recent PRs and a couple of other fixes I want to make. This will make the last in the 5.4.x series. |
|
Excellent, thanks for the update! |
4 participants
Commit 9e9a10a ("Fix double shufti's vector end false positive", #325) changed the tail code in shuftiDoubleExecReal to read a full vector from the current pointer (loadu(d)), which can overread past buf_end by up to S-1 bytes. When the buffer ends at a page boundary followed by unmapped memory, this causes a SIGSEGV.
Fix by reading the last S bytes backward from buf_end (matching the approach used in shuftiExecReal), and falling back to memcpy for buffers shorter than one vector width.
Full disclosure - this change is entirely AI generated (but I understand the bug it tries to fix :) So I'm unsure if this is the correct way to fix it, I'm not acquainted with this project enough. Feel free to overtake this PR, or to close it and fix it someway else.
We've hit this crash in one of our apps after upgrading to 5.4.12 and this seems to be the root cause. Due to the bug's nature I wasn't able to verify that this fix eliminates the problem (this is statistical, depends on whether we try to scan buffers that happen to end on a page boundary followed by an unmapped page).
The added test segfaults without the fix, and the code bytes provided by the kernel match the code bytes the kernel reported when our app crashed.