Page MenuHomePhabricator

Do not map read-only data memory sections with EXECUTE flags.
ClosedPublic

Authored by digit on Apr 21 2020, 9:56 AM.

Details

Summary

The code in SectionMemoryManager.cpp unnecessarily maps
read-only data sections with the READ+EXECUTE flags. This is
undesirable from a security stand-point.

Moreover, on the Fuchsia platform, which is now very strict
about mapping pages with the EXECUTE permission, this simply
fails, because the section's pages were initially allocated
with only the READ+WRITE flags.

A more detailed description of the issue can be found in this
public SwiftShader bug:

  
https://issuetracker.google.com/issues/154586551

This patch just restrict the mapping to the READ flag for ROData
sections. Code sections are still mapped with READ+EXECUTE as
expected.

Diff Detail

Event Timeline

digit created this revision.Apr 21 2020, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 9:56 AM
capn added a subscriber: capn.Apr 21 2020, 1:14 PM
digit added a comment.Apr 22 2020, 2:22 AM

Being very new here, I'm not sure I understand the build failures reported here, since it looks like they are entirely unrelated to this patch.
Is there anything I should do to fix this? retry the build? or just wait?

digit added a comment.Apr 23 2020, 1:12 PM

Friendly ping?

capn added a comment.May 4 2020, 8:24 AM

Ping. Would really love to know whether this execute permission serves a purpose or whether it's a copy-paste bug.

sanjoy removed a reviewer: sanjoy.May 5 2020, 10:53 PM
sanjoy added a subscriber: andrew.w.kaylor.
sanjoy added a subscriber: sanjoy.May 5 2020, 10:55 PM

Andy Kaylor added this functionality in a commit from 2012. So it is better if Andy reviews the change.

sanjoy removed a subscriber: sanjoy.May 5 2020, 10:55 PM
capn added a comment.Jul 27 2020, 6:12 AM

@andrew.w.kaylor Please take a look at this security issue.

lhames accepted this revision.Jul 29 2020, 8:13 AM

I don't see any reason for RODataMem to be mapped executable. I don't think this will break anything, but if it does the fix would be to update RuntimeDyld to ensure any code that needs executing ends up in CodeMem.

LGTM.

This revision is now accepted and ready to land.Jul 29 2020, 8:13 AM

@digit -- Do you have commit access to land this, or would you like me to commit on your behalf?

This revision was landed with ongoing or failed builds.Aug 5 2020, 1:55 AM
This revision was automatically updated to reflect the committed changes.