This is an archive of the discontinued LLVM Phabricator instance.

Basic MTE stack tagging instrumentation.
ClosedPublic

Authored by eugenis on Jul 3 2019, 4:43 PM.

Details

Summary

Use MTE intrinsics to tag stack variables in functions with
sanitize_memtag attribute.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis created this revision.Jul 3 2019, 4:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 4:43 PM
ostannard added inline comments.Jul 8 2019, 7:16 AM
llvm/lib/Target/AArch64/AArch64StackTagging.cpp
66 ↗(On Diff #207923)

You are using -1 as a sentinel value for non-tagged allocations, I think a comment about that here would be useful.

93 ↗(On Diff #207923)

This is only ever written, and shadowed in tagAlloca and untagAlloca.

115 ↗(On Diff #207923)

Could this just use AllocaInst::getAllocationSizeInBits, which implements the same logic?

129 ↗(On Diff #207923)

Is there a fundamental reason we can't tag non-static allocas? Or could/will that be added by a later patch?

140 ↗(On Diff #207923)

This function isn't used anywhere.

213 ↗(On Diff #207923)

Indentation, are there tabs in this file?

247 ↗(On Diff #207923)

Do we still need to change the alignment for allocas we're not going to tag?

254 ↗(On Diff #207923)

This is more of a suggestion for future improvement, doesn't need to be fixed in this patch:

I think picking consecutive tags could result in the same tag for two adjacent allocations, if exactly one tag is disabled by GCR_EL1.Exclude (which I expect to be common, to reserve one tag for otherwise un-tagged memory. In this case, it is likely that the tag picked by "ADDG ... #15" and "ADDG ... #0" will be the same, because the former will have skipped over the excluded tag, resulting in an addition of 16 % 16 == 0.

One option would be to change this to "(NextTag + 1) % 14" (exact value to be bike-shedded), which would avoid generating adjacent tags if up to two bits are set in CR_EL1.Exclude.

Do you happen to know what the linux kernel (or any other OS) plans to set GCR_EL1.Exclude to?

268 ↗(On Diff #207923)

I think this could just use getAnalysis, since we're going to compute it anyway if it's not available.

eugenis marked 2 inline comments as done.Jul 12 2019, 2:49 PM
eugenis added inline comments.
llvm/lib/Target/AArch64/AArch64StackTagging.cpp
254 ↗(On Diff #207923)

One option would be to change this to "(NextTag + 1) % 14"

We could do something fancy like this:
0, 1, ..., 7, 15, 14, ..., 8, (repeat).
This way adjacent tags with never math up to 8 excluded tags.

But I have plans to reorder stack slots in the backend to put more of them within ADDG immediate range from the base tagged pointer. That would make the results of this change less predictable.

Do you happen to know what the linux kernel (or any other OS) plans to set GCR_EL1.Exclude to?

Either 0 or 1 (this thread's SP tag).

268 ↗(On Diff #207923)

In the old pass manager, that would require registering DT as a dependency in getAnalysisUsage, which would cause it to be recomputed every time, even if the function does not have the sanitize_memtag attribute.

eugenis updated this revision to Diff 210008.Jul 15 2019, 6:08 PM
eugenis marked 6 inline comments as done.

address review comments

eugenis marked an inline comment as done.Jul 15 2019, 6:11 PM
eugenis added inline comments.
llvm/lib/Target/AArch64/AArch64StackTagging.cpp
129 ↗(On Diff #207923)

It will be done in a later patch. Added a FIXME.

140 ↗(On Diff #207923)

removed

213 ↗(On Diff #207923)

Indeed, that was a tab. Reformatted the entire file.

247 ↗(On Diff #207923)

I've uploaded the new version that pads allocas with [N x i8] in addition to realigning it.
This way, I believe, untagged allocas do not need to be aligned.

Padding is necessary anyway because STG is technically a memory access of size 16; if alloca size is less than that, BasicAA may conclude that this is impossible and they do not alias.

ostannard accepted this revision.Jul 16 2019, 1:55 AM

LGTM, thanks

This revision is now accepted and ready to land.Jul 16 2019, 1:55 AM
ormris removed a subscriber: ormris.Jul 16 2019, 9:22 AM
This revision was automatically updated to reflect the committed changes.