This is an archive of the discontinued LLVM Phabricator instance.

[MTE] Add -fsanitize=memtag* and friends.
ClosedPublic

Authored by hctim on Feb 3 2022, 2:34 PM.

Details

Summary

Currently, enablement of heap MTE on Android is specified by an ELF note, which
signals to the linker to enable heap MTE. This change allows
-fsanitize=memtag-heap to synthesize these notes, rather than adding them
through the build system. We need to extend this feature to also signal the
linker to do special work for MTE globals (in future) and MTE stack (currently
implemented in the toolchain, but not implemented in the loader).

Current Android uses a non-backwards-compatible ELF note, called
".note.android.memtag". Stack MTE is an ABI break anyway, so we don't mind that
we won't be able to run executables with stack MTE on Android 11/12 devices.

The current expectation is to support the verbiage used by Android, in
that "SYNC" means MTE Synchronous mode, and "ASYNC" effectively means
"fast", using the Kernel auto-upgrade feature that allows
hardware-specific and core-specific configuration as to whether "ASYNC"
would end up being Asynchronous, Asymmetric, or Synchronous on that
particular core, whichever has a reasonable performance delta. Of
course, this is platform and loader-specific.

Diff Detail

Unit TestsFailed

Event Timeline

hctim created this revision.Feb 3 2022, 2:34 PM
hctim requested review of this revision.Feb 3 2022, 2:34 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 3 2022, 2:34 PM

I haven't investigated the use case yet, just commented a few things. Please split the patch into 3:

  • BinaryFormat/ELF.h (see an inline comment)
  • lld/ELF
  • clang
lld/ELF/SyntheticSections.h
1235 ↗(On Diff #405798)

keep variable names alphabetically

MaskRay added inline comments.Feb 3 2022, 3:22 PM
lld/ELF/SyntheticSections.cpp
3842 ↗(On Diff #405798)
3846 ↗(On Diff #405798)

See GnuPropertySection::writeTo

lld/test/ELF/memtag-android-abi.s
1 ↗(On Diff #405798)

REQUIRES: aarch64

Use # for comments. Rename this to aarch64-*

19 ↗(On Diff #405798)

use llvm-readelf -x .note.android.memtag to dump the bytes of a section.

If this is a note section, consider llvm-readelf -n

llvm/include/llvm/BinaryFormat/ELF.h
1536 ↗(On Diff #405798)

If Android wants to define notes, can it use a namespace NT_ANDROID_* like GNU?

Please see https://reviews.llvm.org/D107949 how I add ELF attributes and related tests when new additions are added to BinaryFormat/ELF.h

MaskRay added inline comments.Feb 3 2022, 3:24 PM
lld/ELF/SyntheticSections.cpp
3871 ↗(On Diff #405798)

/*namesz=*/

lld/ELF/SyntheticSections.h
1195 ↗(On Diff #405798)

/*alignment=*/4

lld/ELF/Writer.cpp
367 ↗(On Diff #405798)
hctim marked 9 inline comments as done.Feb 3 2022, 5:40 PM
hctim added inline comments.
llvm/include/llvm/BinaryFormat/ELF.h
1536 ↗(On Diff #405798)

Android currently defines their internal note types in this naming scheme (https://cs.android.com/android/platform/superproject/+/master:bionic/libc/private/bionic_asm_note.h).

However, it seems like these are not used outside of Android internally, so let me slap some prefixes on them. Maybe I'll even get around to updating them in the Android tree as well, who knows.

hctim updated this revision to Diff 405839.Feb 3 2022, 5:40 PM
hctim marked an inline comment as done.

Address Ray's comments, and made it so that stack MTE doesn't imply heap.

hctim added a comment.Feb 3 2022, 5:46 PM

I haven't investigated the use case yet, just commented a few things. Please split the patch into 3:

  • BinaryFormat/ELF.h (see an inline comment)
  • lld/ELF
  • clang

Can you clarify what you'd like (from D107949) in patch #1?

hctim updated this revision to Diff 405842.Feb 3 2022, 5:47 PM

(forgot to add REQUIRES: aarch64 to the lld test)

eugenis added inline comments.Feb 8 2022, 11:16 AM
clang/include/clang/Basic/DiagnosticDriverKinds.td
484–485

Split out renaming of memtag to memtag-stack first? This will remove a lot of diff from this patch.

clang/include/clang/Driver/Options.td
1646

Let's make "sync" the default mode.

lld/ELF/Config.h
346 ↗(On Diff #405842)

NT_MEMTAG_DEFAULT -> NT_MEMTAG_LEVEL_NONE

lld/ELF/Driver.cpp
691 ↗(On Diff #405842)

Should it be required? We could assume default Sync here the same as in clang frontend, that's one less flag to pass around or list in manual linker invocations.

I haven't investigated the use case yet, just commented a few things. Please split the patch into 3:

  • BinaryFormat/ELF.h (see an inline comment)
  • lld/ELF
  • clang

Can you clarify what you'd like (from D107949) in patch #1?

For patch #1, make yaml2obj support the NT_* tags (grep NT_ llvm-readobj/ELF/note-*.test) and obj2yaml able to dump the tag. Add llvm-readobj support.

llvm-objdump support may be unneeded.

MaskRay added inline comments.Feb 8 2022, 10:34 PM
lld/ELF/Driver.cpp
690 ↗(On Diff #405842)

lld uses the diagnostic format specified by https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

Please see other error and warn messages. No capitalization. No trailing perioid.

lld/ELF/Options.td
728 ↗(On Diff #405842)

B/Eq is for legacy options which accept single-dash forms. Newer options should use BB and EEq.

lld/ELF/SyntheticSections.h
1195 ↗(On Diff #405798)

no space after /*

lld/test/ELF/aarch64-memtag-android-abi.s
3 ↗(On Diff #405842)

In lld and llvm binary utilities, we use ## for non-RUN-non-CHECK comments.

hctim planned changes to this revision.Feb 9 2022, 3:27 PM
hctim marked 8 inline comments as done.

Pushed the [1/3] ELF note parsing over to D119381. Will leave this review for the clang-side changes, and send a new patch for the LLD changes. All of your comments are completed in my internal branch, and I'll upload that after the lld changes are landed.

clang/include/clang/Basic/DiagnosticDriverKinds.td
484–485

splitting into elf -> lld -> clang as per Ray's suggestion, should reduce the diff enough.

hctim updated this revision to Diff 413625.Mar 7 2022, 1:51 PM
hctim marked an inline comment as done.

Rebase onto D119384, removing all the llvm and lld-specific parts. This patch now only contains the clang-specific bits.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 1:51 PM
MaskRay added inline comments.Mar 11 2022, 10:02 PM
clang/include/clang/Driver/SanitizerArgs.h
67
clang/lib/Driver/SanitizerArgs.cpp
657

Prefer the more common == to equals

clang/lib/Driver/ToolChains/CommonArgs.cpp
1015
1017
clang/test/Driver/memtag-ld.c
2

Pure driver tests (-###) generally don't need REQUIRES: aarch64-registered-target. You can test that the test should still pass by removing AArch64 from LLVM_TARGETS_TO_BUILD

clang/test/Lexer/has_feature_memtag_heap.cpp
1 ↗(On Diff #413625)

The file can be merged into has_feature_memtag_stack.cpp. Just rename one MemTagSanitizerEnabled to avoid duplicate.

clang/test/Lexer/has_feature_memtag_stack.cpp
2 ↗(On Diff #413625)

%clang is generally only used in test/Driver tests. (There are violations but the patch should not add more).

hctim marked 7 inline comments as done.Apr 1 2022, 3:24 PM
hctim added inline comments.
clang/test/Lexer/has_feature_memtag_stack.cpp
2 ↗(On Diff #413625)

It looks like the SanitizerGroups aren't parsed in in the cc1 invocation, given that using -cc1 provides an error of:

error: invalid value 'memtag' in '-fsanitize='

non-cc1 invocations are thus standard (see clang/test/Lexer/has_feature_undefined_behavior_sanitizer.cpp for an example) for testing sanitizers.

(fwiw - I couldn't find out *why* there's a discrepancy between sanitizers and sanitizer groups, otherwise we could potentially fix it, but it would only be for the sake of "making tests all use _cc1")

hctim updated this revision to Diff 419874.Apr 1 2022, 3:27 PM
hctim marked an inline comment as done.

(comments)

MaskRay added inline comments.Apr 1 2022, 7:33 PM
clang/test/CodeGen/memtag-attr.cpp
13

By placing a space before HasSanitizeMemTag and placing a (, we test that the function name is exact.

clang/test/Driver/memtag-ld.c
37

This will be easier to read if you make the strings after : aligned.

Is -memtag-mode=async --android-memtag-mode=async now?

39

Prefer CHECK-HEAP-SAME to CHECK-HEAP whenever feasible.

hctim updated this revision to Diff 420274.Apr 4 2022, 12:35 PM
hctim marked 2 inline comments as done.

(comments)

hctim marked an inline comment as done.Apr 4 2022, 12:35 PM
hctim added inline comments.
clang/test/Driver/memtag-ld.c
37

thanks for the catch

39

There's no other CHECK-HEAP before this, so -SAME not possible

tschuett added a subscriber: tschuett.EditedApr 4 2022, 12:42 PM

Is -fsanitize=memtag-heap Android specific or target independent? It passes Android flags to the linker?!?

I missed the IsAndroid checks. Sorry.

hctim marked an inline comment as done.Apr 4 2022, 12:55 PM

Is -fsanitize=memtag-heap Android specific or target independent? It passes Android flags to the linker?!?

The frontend flag should be target-independent. The clang driver knows to only pass Android flags to the linker where TC.getTriple().isAndroid() is true. This should allow a glibc-based implementation to live under the same -fsanitize=memtag-heap flag, just chainging the implementation based on the target.

MaskRay added a comment.EditedApr 4 2022, 4:45 PM

Is -fsanitize=memtag-heap Android specific or target independent? It passes Android flags to the linker?!?

The frontend flag should be target-independent. The clang driver knows to only pass Android flags to the linker where TC.getTriple().isAndroid() is true. This should allow a glibc-based implementation to live under the same -fsanitize=memtag-heap flag, just chainging the implementation based on the target.

Since this driver option has no-op for glibc/musl/other ELF OS, by convention it should report an err_drv_unsupported_opt_for_target error.
This allows configure-time detection.

hctim added a comment.Apr 5 2022, 11:52 AM

Since this driver option has no-op for glibc/musl/other ELF OS, by convention it should report an err_drv_unsupported_opt_for_target error.
This allows configure-time detection.

Done.

MaskRay accepted this revision.Apr 7 2022, 10:18 PM
MaskRay added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
484–485

The convention is to omit period for the last sentence.

clang/include/clang/Driver/Options.td
1646

Omit period for the last sentence

clang/lib/Driver/ToolChains/CommonArgs.cpp
1013
clang/test/Lexer/has_feature_memtag.cpp
5
This revision is now accepted and ready to land.Apr 7 2022, 10:18 PM
hctim marked 3 inline comments as done.Apr 8 2022, 10:34 AM
hctim updated this revision to Diff 421581.Apr 8 2022, 10:34 AM

Comments

hctim added a comment.Apr 8 2022, 10:34 AM

Over to @eugenis for any final touches

eugenis accepted this revision.Apr 8 2022, 10:58 AM

LGTM

clang/include/clang/Basic/DiagnosticDriverKinds.td
484–485

Mention armv9a, too?
Something like "Try compiling with -march=armv9a, or -march=armv8a+memtag"

hctim updated this revision to Diff 421593.Apr 8 2022, 11:08 AM
hctim marked an inline comment as done.

.

clang/include/clang/Basic/DiagnosticDriverKinds.td
484–485

mentioned it similar to armv8, as mentioned we're not 100% clear whether MTE is mandatory on armv9.

This revision was landed with ongoing or failed builds.Apr 8 2022, 12:13 PM
This revision was automatically updated to reflect the committed changes.
hctim added a comment.Apr 8 2022, 1:18 PM

Thanks, I'll take a look.

clang/test/Lexer/has_feature_memtag.cpp