-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cxbe: Fix oob section reads #652
Conversation
tools/cxbe/Exe.cpp
Outdated
|
||
memset(m_bzSection[v], 0, raw_size); | ||
memset(m_bzSection[v], 0, virt_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use the actual allocated size when clearing (i.e., the result of the max
)? At a glance it looks like a potential source of error, even if it's safe in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd just clear more bytes than necessary - virt_size > raw_size
will result in the buffer getting completely zeroed, virt_size < raw_size
will result in the buffer being completely filled with a copy of what's in the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood that it'd have no actual effect, it's more that it looks potentially erroneous so somebody trying to debug a similar issue to the one that led to this fix might waste time thinking that uninitialized section data was in play.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I'd prefer it to be like what the actual loader at runtime does, but I see what you mean. I've changed it, the performance difference is insignificant anyway.
As it's a somewhat critical bugfix and hasn't gotten any more feedback I'll merge it now. |
Fixes regression introduced by d6f0f3a.
The underlying issue is that trailing null bytes are not stored in the exe and thus are not part of
dwSizeofRaw
, causing the null terminator of thexboxkrnl.exe
-string to get cut off resulting in an oob-read and invalid string comparison, which ultimately produces an xbe with an incorrect import directory (thehello
sample can be used to test this).I've changed the exe loader and xbe constructor to instead keep the full section in memory so that valid reads from sections can no longer be out of bounds. This slightly increases memory consumption during the conversion, but that shouldn't be an issue.