This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Driver] validate sysregs for -mstack-protector-guard-reg=
AbandonedPublic

Authored by nickdesaulniers on Apr 26 2021, 2:55 PM.

Details

Summary

In order to support arbitrary AArch64 sysregs for
-mstack-protector-guard-reg=, copy the generated list from
lib/Target/AArch64/AArch64GenSystemOperands.inc's lookupSysRegByName().
This avoids requiring the AArch64 backend to be linked to validate such
registers.

Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Apr 26 2021, 2:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 2:55 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
  • remove comment about string case
MaskRay added inline comments.Apr 26 2021, 8:28 PM
clang/lib/Driver/ToolChains/Arch/AArch64.cpp
1425

We should avoid static constructors/destructors if possible. Just use a plain array and llvm::find(ValidRegs, RegName)

A lot of these system registers are going to be unsuitable for the stack canary. They're either read only, they have fixed bits, or they'll get overwritten by various system events.

I'm not suggesting we audit this list for those things, what seems reasonable to me is just to support the ones that the Linux kernel uses. (what GCC allows I'm not sure)

https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/AArch64-Options.html

-mstack-protector-guard=guard
-mstack-protector-guard-reg=reg
-mstack-protector-guard-offset=offset
Generate stack protection code using canary at guard. Supported locations are ‘global’ for a global canary or ‘sysreg’ for a canary in an appropriate system register.

With the latter choice the options -mstack-protector-guard-reg=reg and -mstack-protector-guard-offset=offset furthermore specify which system register to use as base register for reading the canary, and from what offset from that base register. There is no default register or offset as this is entirely for use within the Linux kernel.

So if kernel builds are using a small subset of registers, just check for those. Otherwise we've got another duplicated list that we (Arm) will probably forget to update anyway when new registers are added.

Either way it's one more stick on the "why doesn't clang just use the backend info" fire, but it's smaller at least if we limit the registers. E.g.

error: invalid value 'foo' in 'mstack-protector-guard-reg=','for AArch64' valid values are sp_el1 sp_el2
  • prefer vanilla array + llvm::find

So if kernel builds are using a small subset of registers, just check for those.

The kernel uses just one for aarch64. It looks like GCC just feeds through whatever is specified at the command line to GAS without any validation; letting GAS fail or not. For reference, here are the ISAs+regs used currently by the Linux kernel.

  • riscv: tp
  • powerpc: r13, r2
  • x86: fs
  • arm64: sp_el0

So it's definitely overkill to validate all possible sysregs. Just thought I'd post a patch in case we ever wanted to revisit this, but whether this patch lands or not doesn't matter to me; https://reviews.llvm.org/D100919 is what we need, hence the separation.

Otherwise we've got another duplicated list that we (Arm) will probably forget to update anyway when new registers are added.

Agreed.

nickdesaulniers marked an inline comment as done.Apr 27 2021, 11:35 AM
nickdesaulniers abandoned this revision.May 17 2021, 11:53 AM

I don't plan to land this; I wrote it in case it comes up in code review. llvm's codegen will report_fatal_error for garbage sysregs. If it comes up again in the future, this phab review will still exist for reference, but I suggest we only add what's needed in that case, not all possible sysregs since most make sense in this context I suspect.