[release/7.0-preview5] Fix VTableCallHolder writeable mapping size with W^X#70103
Merged
carlossanlop merged 1 commit intoJun 2, 2022
Merged
Conversation
The size of this holder is dynamic, but when we are creating the writeable mapping of this holder to initialize its code, we don't take that into account. So in case the holder is located at the end of a memory page and crosses its boundary, the writeable mapping covers only the beginning of the holder and so we either crash during the initialization if the following memory page is not mapped or read only, or we corrupt a completely unrelated memory page in case it is mapped and writeable. The fix is to use the real size of the holder instead of sizeof(...).
ghost
added
the
mangod9
approved these changes
Jun 1, 2022
Member
jeffschwMSFT
approved these changes
Jun 1, 2022
Member
jeffschwMSFT
left a comment
jeffschwMSFT
left a comment
There was a problem hiding this comment.
Approved. We will take for consideration in preview5
carlossanlop
added
Servicing-approved
Contributor
|
Approved by Tactics via email. |
Contributor
|
Test failure is unrelated: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #70093 to release/7.0-preview5
The size of this holder is dynamic, but when we are creating the writeable
mapping of this holder to initialize its code, we don't take that into account.
So in case the holder is located at the end of a memory page and crosses its
boundary, the writeable mapping covers only the beginning of the holder and
so we either crash during the initialization if the following memory page is
not mapped or read only, or we corrupt a completely unrelated memory page
in case it is mapped and writeable.
The fix is to use the real size of the holder instead of sizeof(...).
/cc @janvorli
Customer Impact
Runtime with W^X enabled (the default in preview 5) can rarely crash with AccessViolation exception.
Testing
Coreclr and libraries tests, @333fred has tested a drop-in replacement of a libcoreclr.so with this fix in an installer repo test that was crashing for him in 8 out of 10 cases, with this fix it was passing ok.
Risk
Very low, the change just makes writeable memory mapping larger than before (so that it covers the area of memory that was just allocated for the holder) and we only access memory range of the holder.