Page MenuHomePhabricator

[ARM][AArch64] SLSHardening: make non-comdat thunks possible
ClosedPublic

Authored by danielkiss on Apr 15 2021, 4:18 AM.

Details

Summary

Linker scripts might not handle COMDAT sections. SLSHardeing adds
new section for each __llvm_slsblr_thunk_xN. This new option allows
the generation of the thunks into the normal text section to handle these
exceptional cases.
,comdat or ,noncomdat can be added to harden-sls to control the codegen.
-mharden-sls=[all|retbr|blr],nocomdat.

Diff Detail

Event Timeline

danielkiss created this revision.Apr 15 2021, 4:18 AM
danielkiss requested review of this revision.Apr 15 2021, 4:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 4:18 AM
tamas.petz added inline comments.Apr 15 2021, 4:22 AM
llvm/include/llvm/CodeGen/IndirectThunks.h
56

NIT: tabs to spaces

Apart from maybe needing to run clang-format on the patch, the code changes look good to me.
Before this could be committed, this needs tests.

llvm/include/llvm/CodeGen/IndirectThunks.h
43–44

I'm not entirely sure, but is this how clang-format formats these lines?

danielkiss edited the summary of this revision. (Show Details)

Adding a fronted flag and tests.
comdat is still the default, feels fine but I'm not sure.

danielkiss marked 2 inline comments as done.May 10 2021, 9:19 AM
danielkiss added inline comments.
llvm/include/llvm/CodeGen/IndirectThunks.h
43–44

yes, patch went thru git clang-format

I have just a few small comments.

llvm/include/llvm/CodeGen/IndirectThunks.h
31

Is the default argument really required?

57

There are still TABs there.

danielkiss marked an inline comment as done.May 10 2021, 9:34 AM
danielkiss added inline comments.
llvm/include/llvm/CodeGen/IndirectThunks.h
31

yes, this function is called from x86 backed and I'd not change that.

57

I think these are spaces, could you please take a look at the raw diff?

kristof.beyls added inline comments.May 11 2021, 5:57 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
841–844

I think it would be better if comdat/nocomdat were also communicated through Features.
It keeps the interface on this part of the code base consistent (no need to introduce a CmdArgs feature); and also makes sure that LTO vs non-LTO builds keep on behaving the same way, as the feature is set per function, not per translation unit.

I haven't really thought about how to best specify that feature. Maybe just have an extra 2 features: +harden-sls-comdat and +harden-sls-nocomdat?

llvm/test/CodeGen/AArch64/speculation-hardening-sls.ll
218–220

Would it be possible to also have check lines for HARDEN-COMDAT-OFF, i.e. look for lines that HARDEN-COMDAT-OFF should produce?

llvm/test/CodeGen/ARM/speculation-hardening-sls.ll
249–251

Would it be possible to also have check lines for HARDEN-COMDAT-OFF, i.e. look for lines that HARDEN-COMDAT-OFF should produce?

danielkiss added inline comments.May 11 2021, 1:27 PM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
841–844

I almost added a feature for the harden-sls-noncomdat but that is not really a HW feature so felt the backed flag is better.
One feature bit seems enough to describe the two states.

address review comments.

danielkiss marked 5 inline comments as done.May 18 2021, 5:42 AM
kristof.beyls accepted this revision.May 18 2021, 7:48 AM

Thanks @danielkiss !
I only have a few nit picky remarks.
Overall looks good!

clang/lib/Driver/ToolChains/Arch/AArch64.cpp
275

I'm wondering if the '(EnableRetBr || EnableBlr)' condition is needed here?
Just dropping it would make this code a little bit simpler.
I'm assuming here of course that the semantics of the +harden-sls-comdat target feature is to not produce comdat sections in case any of the other sls-hardening target features would produce thunks.
I agree this is a bit of a nit pick though.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
841

Same comment/thought on the "(EnableRetBr || EnableBlr)" condition as on the AArch64 side.

llvm/lib/Target/AArch64/AArch64.td
528

I think it would be more accurate if the word "thunk" would be present in the documentation.
What about the following:
"Generate thunk code for SLS mitigation in the normal text section"?

llvm/lib/Target/ARM/ARM.td
578

Same comment as on the AArch64 side on using the word "thunk" somewhere in the help text.

This revision is now accepted and ready to land.May 18 2021, 7:48 AM
danielkiss marked 4 inline comments as done.
danielkiss added inline comments.
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
275

the check is a bit overkill, the backend won't do anything anyway without the EnableRetBr || EnableBlr

jcai19 added a subscriber: jcai19.May 19 2021, 9:44 AM
This revision was landed with ongoing or failed builds.May 20 2021, 8:07 AM
This revision was automatically updated to reflect the committed changes.
danielkiss marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2021, 8:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript