This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Add Scudo support for Trusty OS
ClosedPublic

Authored by danieljm on Jun 2 2021, 5:33 PM.

Details

Summary

trusty.cpp and trusty.h define Trusty implementations of map and other
platform-specific functions. In addition to adding Trusty configurations
in allocator_config.h and size_class_map.h, MapSizeIncrement and
PrimaryEnableRandomOffset are added as configurable options in
allocator_config.h.
Background on Trusty: https://source.android.com/security/trusty

Diff Detail

Event Timeline

danieljm created this revision.Jun 2 2021, 5:33 PM
danieljm requested review of this revision.Jun 2 2021, 5:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2021, 5:33 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
danieljm added a subscriber: trong.Jun 2 2021, 5:37 PM
vitalybuka added inline comments.
compiler-rt/lib/scudo/standalone/memtag.h
58–60 ↗(On Diff #349430)

Maybe

compiler-rt/lib/scudo/standalone/trusty.cpp
63–67

some tests should fails without reasonable implementation
what is the plan for testing?

Thank you Daniel, this looks like a good start!
I'll work on my side on reducing some of the memory footprint of some structures and whatnot (scoped strings come to mind).
As discussed, we can re-introduce the use-separate-class-for-batches distinction for Trusty, which can be a large win to get rid of a class size.
I still think the Primary32 might be better suited here, but we'll follow up.

compiler-rt/lib/scudo/standalone/allocator_config.h
48

I think this should be PrimaryMapSizeIncrement to be homogeneous with the naming of all the Primary settings.

compiler-rt/lib/scudo/standalone/primary64.h
79

Maybe use a ternary here?
Region->RegionBeg = getRegionBaseByClassId(I) + (PrimaryEnableRandomOffset ? getRandomModN(&Seed, 16) + 1) * PageSize : 0);

275

If it's only used once, you might just want to use Config::PrimaryEnableRandomOffset and not add the extra definition.

compiler-rt/lib/scudo/standalone/trusty.cpp
34

Addr is UNUSED

37

The variables in this function don't have the "proper" casing. Should be ProgramBreak and so on.

danieljm updated this revision to Diff 349723.Jun 3 2021, 4:11 PM

Updated scudo/standalone/tests with primary64 configurable
options PrimaryEnableRandomOffset and PrimaryMapSizeIncrement.

danieljm marked 7 inline comments as done.Jun 3 2021, 4:18 PM
danieljm added inline comments.
compiler-rt/lib/scudo/standalone/memtag.h
58–60 ↗(On Diff #349430)

Even if I make this suggested change, there is non-Linux-specific AArch64 assembly which cannot be compiled with Trusty's build system (see lines 119-265).

compiler-rt/lib/scudo/standalone/trusty.cpp
63–67

Trusty has its own build system, so the existing Scudo tests would need to be ported to Trusty as test apps. We have a Trusty test app for Scudo running downstream on the Trusty side, and we can consider porting other Scudo tests to Trusty downstream.

danieljm updated this revision to Diff 349734.Jun 3 2021, 5:50 PM
danieljm marked 2 inline comments as done.

Passed primary64.h through clang-format.

pcc added a subscriber: pcc.Jun 3 2021, 6:26 PM
pcc added inline comments.
compiler-rt/lib/scudo/standalone/memtag.h
58–60 ↗(On Diff #349430)

Isn't it a compiler issue rather than a build system issue? Can you resolve it by upgrading the compiler? (I think we technically should be making this conditional on the compiler version since our minimum supported compiler version is older than the version that added MTE support, but that's a separate issue.)

danieljm updated this revision to Diff 349876.Jun 4 2021, 7:52 AM

Reversion of changes to memtag.h and mutex.h

danieljm marked an inline comment as done.Jun 4 2021, 7:54 AM
danieljm added inline comments.
compiler-rt/lib/scudo/standalone/memtag.h
58–60 ↗(On Diff #349430)

You're right, I meant the Trusty compiler was out of date. As it turns out, the compiler was just updated so now this change is unnecessary and memtag.h can be left as it was before.

cryptoad added inline comments.Jun 4 2021, 7:57 AM
compiler-rt/lib/scudo/standalone/trusty.cpp
93

Isn't printf using malloc (or others)? If so this will recurse.

cryptoad added inline comments.Jun 4 2021, 8:00 AM
compiler-rt/lib/scudo/standalone/trusty.cpp
52

Can _trusty_brk set the errno to ENOMEM?

danieljm updated this revision to Diff 349909.Jun 4 2021, 10:11 AM
danieljm marked 2 inline comments as done.

set errno manually in map since _trusty_brk does not set it

danieljm marked an inline comment as done.Jun 4 2021, 10:14 AM
danieljm added inline comments.
compiler-rt/lib/scudo/standalone/trusty.cpp
52

It turns out that _trusty_brk does not set errno, so I modify map to set it manually. This is consistent with how Trusty implements sbrk using _trusty_brk as well. Also, since we know errno equals ENOMEM in this if branch, we just directly call dieOnMapUnmapError(Size) instead of dieOnMapUnmapError(errno == ENOMEM ? Size : 0);

93

Trusty uses Musl's implementation of printf which uses a fixed size buffer allocated on the stack.

danieljm updated this revision to Diff 350339.Jun 7 2021, 9:51 AM
danieljm marked an inline comment as done.

Rebased patch.

cryptoad accepted this revision.Jun 7 2021, 11:10 AM

I patched it locally and everything passes for me.
Given the current timeline, I am OK with landing it now as is, and improve gradually on it.
I will pick up some of the action items, notably to try and reduce the memory footprint.

This revision is now accepted and ready to land.Jun 7 2021, 11:10 AM
vitalybuka added inline comments.Jun 7 2021, 11:19 AM
compiler-rt/lib/scudo/standalone/allocator_config.h
172

Please add corresponding SCUDO_TYPED_TEST_TYPE(FIXTURE, NAME, TrustyConfig) into combined test

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
112

SCUDO_TYPED_TEST_TYPE(FIXTURE, NAME, TrustyConfig)

cryptoad added inline comments.Jun 7 2021, 1:13 PM
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
112

Enabling the Trusty config on non-trusty config is wonky:

#4  0x0000555555438f23 in scudo::map (Addr=0x7fffcfaab400, Size=1024, 
    Name=0x7ffff7a68237 "scudo:primary", Flags=5, Data=0x172aff200210)
    at third_party/llvm/llvm-project/compiler-rt/lib/scudo/standalone/linux.cpp:69

It ends up trying to map at a non-page boundary a non-multiple-of-page size, ending up in an abort.

vitalybuka accepted this revision.Jun 8 2021, 10:03 AM

LGTM but upstream testing could be very helpful for those who change code without TrustyOS setup

This revision was landed with ongoing or failed builds.Jun 8 2021, 2:02 PM
This revision was automatically updated to reflect the committed changes.