This is an archive of the discontinued LLVM Phabricator instance.

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

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

Download Raw Diff

Details

Reviewers

loladiro
lhames

Commits

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

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

Unit TestsFailed

	Time	Test
	11,910 ms	lldb-api.commands/expression/import-std-module/basic::Unknown Unit Message ("")
	3,600 ms	lldb-api.commands/expression/import-std-module/conflicts::Unknown Unit Message ("")
	6,060 ms	lldb-api.commands/expression/import-std-module/deque-basic::Unknown Unit Message ("")
	5,820 ms	lldb-api.commands/expression/import-std-module/deque-dbg-info-content::Unknown Unit Message ("")
	5,380 ms	lldb-api.commands/expression/import-std-module/forward_list::Unknown Unit Message ("")
		View Full Test Results (41 Failed)

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

Herald added subscribers: llvm-commits, hiraditya. · View Herald Transcript

Harbormaster failed remote builds in B54115: Diff 259038!Apr 21 2020, 10:48 AM

capn added a subscriber: capn.Apr 21 2020, 1:14 PM

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?

ben-clayton added a subscriber: ben-clayton.Apr 22 2020, 8:52 AM

amaiorano added a subscriber: amaiorano.Apr 22 2020, 12:24 PM

Friendly ping?

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.

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

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

gribozavr2 added a reviewer: lhames.Jul 29 2020, 6: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

Closed by commit rGba0e71432a60: Do not map read-only data memory sections with EXECUTE flags. (authored by digit, committed by bkramer). · Explain Why

This revision was automatically updated to reflect the committed changes.

bkramer added a commit: rGba0e71432a60: Do not map read-only data memory sections with EXECUTE flags..

Revision Contents

Path

Size

llvm/

lib/

ExecutionEngine/

SectionMemoryManager.cpp

3 lines

Diff 259038

llvm/lib/ExecutionEngine/SectionMemoryManager.cpp

Show First 20 Lines • Show All 146 Lines • ▼ Show 20 Lines	bool SectionMemoryManager::finalizeMemory(std::string *ErrMsg) {
if (ec) {		if (ec) {
if (ErrMsg) {		if (ErrMsg) {
*ErrMsg = ec.message();		*ErrMsg = ec.message();
}		}
return true;		return true;
}		}

// Make read-only data memory read-only.		// Make read-only data memory read-only.
ec = applyMemoryGroupPermissions(RODataMem,		ec = applyMemoryGroupPermissions(RODataMem, sys::Memory::MF_READ);
sys::Memory::MF_READ \| sys::Memory::MF_EXEC);
if (ec) {		if (ec) {
if (ErrMsg) {		if (ErrMsg) {
*ErrMsg = ec.message();		*ErrMsg = ec.message();
}		}
return true;		return true;
}		}

// Read-write data memory already has the correct permissions		// Read-write data memory already has the correct permissions
▲ Show 20 Lines • Show All 103 Lines • Show Last 20 Lines