This is an archive of the discontinued LLVM Phabricator instance.

[ARM] implement support for TLS register based stack protector
ClosedPublic

Authored by ardb on Oct 28 2021, 4:09 PM.

Details

Summary

Implement support for loading the stack canary from a memory location held in
the TLS register, with an optional offset applied. This is used by the Linux
kernel to implement per-task stack canaries, which is impossible on SMP systems
when using a global variable for the stack canary.

Suggestions for how to test this and where to insert the test code into the
tree are kindly welcomed.

Diff Detail

Event Timeline

ardb created this revision.Oct 28 2021, 4:09 PM
ardb requested review of this revision.Oct 28 2021, 4:09 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 28 2021, 4:09 PM

Nice job! This looks like it hits every place that I was looking at for this.

In terms of tests, https://reviews.llvm.org/D100919 and https://reviews.llvm.org/D102742 are probably interesting.

In particular, we should test that clang no longer rejects -mstack-protector-guard=tls -mstack-protector-guard-offset=0 for --target=arm-linux-gnueabihf and --target=arm-linux-gnueabihf -mthumb. (clang/test/Driver/stack-protector-guard.c
)

Then we should test

target triple = "arm-unknown-linux-gnueabihf"

declare void @baz(i32*)

define void @foo(i64 %t) sspstrong {
  %vla = alloca i32, i64 %t, align 4
  call void @baz(i32* nonnull %vla)
  ret void
}
!llvm.module.flags = !{!1, !2}
!1 = !{i32 2, !"stack-protector-guard", !"tls"}
!2 = !{i32 2, !"stack-protector-guard-offset", !"0"}

generates mrc (and test again with "thumbv7-linux-gnu"). We can use -mtriple= flags to llc rather than hard coding the triple in IR. (create llvm/test/CodeGen/ARM/stack-guard-sysreg.ll)

This also involves implementing LOAD_STACK_GUARD for other ARM targets than
Mach-O.

It might be worthwhile to break this up into 2 commits; one that does just that, so that we can isolate any test changes or possible build breakages to that, then have this patch implementing support for tls stack guards on top.

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
4903

I think we want to use Reg in this instruction, in order to load into the specified destination?

4907

do we need to add al?

llvm/lib/Target/ARM/ARMInstrInfo.cpp
102

do we have to worry about soft tp, ie. __aeabi_read_tp vs mrc?

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
250

what about Thumb1InstrInfo::expandLoadStackGuard? Do we have mrc available in Thumb[1] as well?

ardb added a comment.Oct 29 2021, 7:28 AM

I have split off the LOAD_STACK_GUARD changes into

[ARM] implement LOAD_STACK_GUARD for remaining targets
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
4903

We use Reg, no? MRC does not take reg operands, but I pass it as the target register.

4907

It appears so.

llvm/lib/Target/ARM/ARMInstrInfo.cpp
102

I think we should only allow this hard TP is supported in the first place. I'll add a check somewhere.
Calling a helper in both the prologue and the epilogue for emulated TLS seems a bit too heavyweight to me, and the Linux kernel will not use it in that way anyway.

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
250

No Thumb1 does not have an encoding for MRC so we can ignore it here. It does mean we should not allow this feature to be turned out for such targets.

ardb updated this revision to Diff 383390.Oct 29 2021, 9:29 AM
ardb edited the summary of this revision. (Show Details)
  • split off LOAD_STACK_GUARD conversion
  • deal with guard offsets >= 4096 bytes
  • reject offsets < 0 or >= 1 MiB
  • add backend test to check that the MRC/LDR sequence is emitted twice

Codegen looks good, probably just need an additional conditional on ARMSubtarget::isReadTPHard(). With that and some more tests for cases we don't intend to support (thumb[1], soft tp) this LGTM. Great job @ardb !

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
4903

ah, yep, sorry, LGTM.

llvm/lib/Target/ARM/ARMInstrInfo.cpp
102

I agree. Let's add a test that this only works with the -mattr=+read-tp-hard llc flag or "target-features"="+read-tp-hard" function attribute (one or the other, I don't think we need to exhaustively test both). But it would be good to verify what happens when +read-tp-hard is not set, in case someone else cares to add soft tp support here in the future. (They may never, it's more so to highlight whether such a change would be intentional).

ARMSubtarget::isReadTPHard() should be able to help us differentiate this case.

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
250

Let's add a test for such case.

llvm/test/CodeGen/ARM/stack-guard-tls.ll
6–12

You can convert --check-prefix=CHECK --check-prefix=CHECK-SMALL to --check-prefixes=CHECK,CHECK-SMALL.

ardb added inline comments.Oct 29 2021, 3:25 PM
llvm/lib/Target/ARM/ARMInstrInfo.cpp
102

I agree. Let's add a test that this only works with the -mattr=+read-tp-hard llc flag or "target-features"="+read-tp-hard" function attribute (one or the other, I don't think we need to exhaustively test both). But it would be good to verify what happens when +read-tp-hard is not set, in case someone else cares to add soft tp support here in the future. (They may never, it's more so to highlight whether such a change would be intentional).

ARMSubtarget::isReadTPHard() should be able to help us differentiate this case.

I could not figure out how to check this at command line parsing time. If we check it later, we basically crash the compiler, right? Not very elegant ...

llvm/lib/Target/ARM/Thumb2InstrInfo.cpp
250

Let's add a test for such case.

Same problem: I could not figure out how to decide early enough whether we are targetting Thumb1 or Thumb2/ARM

Adding ostannard to reviewers list. I'm going to be on vacation next week and Oliver is more familiar with this area than I am.

To prevent the option in Clang for targets that don't support Thumb2 it may be worth looking into clang/lib/Basic/Targets/ARM.cpp for example https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Targets/ARM.cpp#L174

ardb updated this revision to Diff 384116.Nov 2 2021, 8:35 AM
  • add diagnostics to the frontend and asserts to the backend to ensure that the TLS stack protector is only used on target subarchs that implement the hardware TLS register to begin with
  • ensure that the offset parameter is not omitted, as the default is INT_MAX which is out of bounds
ardb marked 7 inline comments as done.Nov 2 2021, 8:36 AM
ardb updated this revision to Diff 384154.Nov 2 2021, 10:28 AM
  • fix failure in newly added LLVM test
clang/lib/Driver/ToolChains/Clang.cpp
3178–3180

Does GCC require these flags to be paired (-mstack-protector-guard= and -mstack-protector-guard-offset) ? Only for ARM and THUMB? Doesn't the offset only make sense for sysreg and tls, or global, too?

Seems to me like 0 would be a good default offset, if unspecified.

Perhaps this would good to check then warn on for all supported architectures, and perhaps as another child patch? (or just default to 0 and not require -mstack-protector-offset=).

3191–3192

Isn't this redundant/set elsewhere?

ardb added inline comments.Nov 2 2021, 11:37 AM
clang/lib/Driver/ToolChains/Clang.cpp
3178–3180

Does GCC require these flags to be paired (-mstack-protector-guard= and -mstack-protector-guard-offset) ? Only for ARM and THUMB? Doesn't the offset only make sense for sysreg and tls, or global, too?

GCC does not implement this for ARM yet. For AArch64, it requires all three options: guard, guard-reg and guard-offset.

Seems to me like 0 would be a good default offset, if unspecified.

The default is INT_MAX at the moment, and I wasn't sure if we can simply change that without breaking users.

Perhaps this would good to check then warn on for all supported architectures, and perhaps as another child patch? (or just default to 0 and not require -mstack-protector-offset=).

Yes, I think it makes sense to check this for AAch64 as well.

3191–3192

No, it is not. The alternative is requiring the user to pass -mtp=cp15 when using the TLS stack protector, which is silly because it is implied.

clang/lib/Driver/ToolChains/Clang.cpp
3191–3192

Right. Perhaps we could emit diag::err_drv_argument_not_allowed_with if the user specified -mtp=soft with -mstack-protector-guard=tls.

It looks like a call to clang::driver::tools::arm::getReadTPMode() should being able to tell us whether someone explicitly tried to use soft.

ardb updated this revision to Diff 384564.Nov 3 2021, 1:00 PM
  • disallow -mtp=soft when TLS based stack protector is enabled
ardb marked 2 inline comments as done.Nov 3 2021, 1:01 PM
ardb added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
3191–3192

Right. Perhaps we could emit diag::err_drv_argument_not_allowed_with if the user specified -mtp=soft with -mstack-protector-guard=tls.

It looks like a call to clang::driver::tools::arm::getReadTPMode() should being able to tell us whether someone explicitly tried to use soft.

nickdesaulniers accepted this revision.Nov 3 2021, 1:04 PM

Looks great! Nice job!

This revision is now accepted and ready to land.Nov 3 2021, 1:04 PM
This revision was automatically updated to reflect the committed changes.
ardb marked an inline comment as done.