On 1/20/22 04:02, Richard Sandiford wrote:
Thanks for the patch and sorry for the (very) slow review.
Thanks for the review, Richard :).
+/* Handle a "no_sanitize_shadow_call_stack" attribute; arguments as in
+ struct attribute_spec.handler. */
+static tree
+handle_no_sanitize_shadow_call_stack_attribute (tree *node, tree name,
+ tree, int, bool *no_add_attrs)
+{
+ *no_add_attrs = true;
+ if (TREE_CODE (*node) != FUNCTION_DECL)
+ warning (OPT_Wattributes, "%qE attribute ignored", name);
+ else
+ add_no_sanitize_value (*node, SANITIZE_SHADOW_CALL_STACK);
+
+ return NULL_TREE;
+}
+
Do we need this? I think these days the preference is to use the
general "no_sanitize" attribute with an argument, which is also the
syntax documented on the clang page.
We have to support no_sanitize_foo attributes for some of the early
sanitiser features, to avoid breaking backwards compatibility, but it
doesn't look like clang ever supported no_sanitize_shadow_call_stack.
Oh, "no_sanitize" is enough, I will delete this in the next version.
+/* Return TRUE if shadow call stack should be enabled for the current
+ function, otherwise return FALSE. */
+
+bool
+aarch64_shadow_call_stack_enabled (void)
+{
+ /* This function should only be called after frame laid out. */
+ gcc_assert (cfun->machine->frame.laid_out);
+
+ if (crtl->calls_eh_return)
+ return false;
+
+ /* We only deal with a function if its LR is pushed onto stack
+ and attribute no_sanitize_shadow_call_stack is not specified. */
(This would need to be updated if we do drop the specific attribute.)
Ok.
+ /* Push return address to shadow call stack. */
+ if (aarch64_shadow_call_stack_enabled ())
+ emit_insn (gen_scs_push ());
Formatting nit: should only be indented by two spaces more than the “if”.
Same below.
Got it.
I guess doing it like this means that we also continue to store x30 to the
frame in the traditional way. And that's probably necessary, given that
the saved x30 forms part of link chain.
Yes, we just added an extra insn to the prologue like:
+ str x30, [x18], #8
stp x29, x30, [sp, #-16]!
.......
+
if (flag_stack_usage_info)
current_function_static_stack_size = constant_lower_bound (frame_size);
@@ -9066,6 +9089,10 @@ aarch64_expand_epilogue (bool for_sibcall)
RTX_FRAME_RELATED_P (insn) = 1;
}
+ /* Pop return address from shadow call stack. */
+ if (aarch64_shadow_call_stack_enabled ())
+ emit_insn (gen_scs_pop ());
+
This looks correct, but following on from the above, I guess we continue
to restore x30 from the frame in the traditional way first, and then
overwrite it with the SCS value. I think the output code would be
slightly neater if we suppressed the first restore of x30.
Yes, the current epilogue is like:
.......
ldp x29, x30, [sp], #16
+ ldr x30, [x18, #-8]!
I think may be we can keep the two x30 pops here, for two reasons:
1) The implementation of scs in clang is to pop x30 twice, it might be
better to be consistent with clang here[1].
2) If we keep the first restore of x30, it may provide some flexibility
for other scenarios. For example, we can directly patch the scs_push/pop
insns in the binary to turn it into a binary without scs;
[1]
https://github.com/llvm/llvm-project/commit/f11eb3ebe77729426e562d7d4d7ebb1d5ff2e7c8
+/* This value represents whether the shadow call stack is implemented on
+ the target platform. */
+#define TARGET_SUPPORT_SHADOW_CALL_STACK true
+
+#define TARGET_CHECK_SCS_RESERVED_REGISTER() \
+ do { \
+ if (!fixed_regs[R18_REGNUM]) \
+ error ("%<-fsanitize=shadow-call-stack%> only " \
+ "allowed with %<-ffixed-x18%>"); \
+ } while (0)
+
Please make these target hooks instead. The first one can use DEFHOOKPOD.
Ok, I will add a field to gcc_target via DEFHOOKPOD.
+;; Save X30 in the X18-based POST_INC stack (consistent with clang).
+(define_insn "scs_push"
+ [(set (mem:DI (reg:DI R18_REGNUM)) (reg:DI R30_REGNUM))
+ (set (reg:DI R18_REGNUM) (plus:DI (reg:DI R18_REGNUM) (const_int 8)))]
+ ""
+ "str\\tx30, [x18], #8"
+ [(set_attr "type" "store_8")]
+)
+
+;; Load X30 form the X18-based PRE_DEC stack (consistent with clang).
+(define_insn "scs_pop"
+ [(set (reg:DI R18_REGNUM) (minus:DI (reg:DI R18_REGNUM) (const_int 8)))
+ (set (reg:DI R30_REGNUM) (mem:DI (reg:DI R18_REGNUM)))]
+ ""
+ "ldr\\tx30, [x18, #-8]!"
+ [(set_attr "type" "load_8")]
+)
+
The two SETs here work in parallel, with the define_insn as a whole
following a "read input operands, act, write output operands" sequence.
The (mem: …) address therefore sees the old value of r18 rather than the
new one. Also, RTL rules say that subtractions need to be written as
additions of the negative.
Oh, sorry, I got it wrong here.
I think the pattern would therefore be something like:
[(set (reg:DI R30_REGNUM) (mem:DI (plus:DI (reg:DI R18_REGNUM)
(const_int -8))))]
(set (reg:DI R18_REGNUM) (plus:DI (reg:DI R18_REGNUM) (const_int -8)))]
However, since these are normal moves, I think we should be able to use
the standard move patterns. E.g. the push could be:
(define_expand "scs_push"
[(set (mem:DI (pre_dec:DI (reg:DI R18_REGNUM)))
(reg:DI R30_REGNUM))])
and similarly for the pop.
Thanks, this template looks better.
Since the push/pop of SCS and normal stack in clang are different
(for example, scs push is post_inc, while normal stack is pre_dec),
in order to maintain consistency, I think it can be changed to the
following:
(define_expand "scs_push"
[(set (mem:DI (post_inc:DI (reg:DI R18_REGNUM)))
(reg:DI R30_REGNUM))])
(define_expand "scs_pop"
[(set (reg:DI R30_REGNUM)
(mem:DI (pre_dec:DI (reg:DI R18_REGNUM))))])
+Enable ShadowCallStack, a security enhancement mechanism used to protect
+programs against return address overwrites (e.g. stack buffer overflows.)
+It works by saving a function's return address to a separately allocated
+shadow call stack in the function prologue and restore the return address
typo: s/restore/restoring/
Got it.
+from the shadow call stack in the function epilogue. Instrumentation only
+occurs in functions that need to save the return address to the stack.
+
+Currently it only supports the aarch64 platform and only works fine in
+the linux kernel that implements CONFIG_SHADOW_CALL_STACK.
Maybe:
Currently it only supports the aarch64 platform. It is specifically
designed for linux kernels that enable the CONFIG_SHADOW_CALL_STACK option.
Ok.
+For the user space programs, runtime support is not currently provided
+in libc and libgcc. Users who want to use this feature in user space need
+to provide their own support for the runtime. It should be noted that
+this may cause the ABI rules to be broken.
+
+On aarch64, the instrumentation makes use of the platform register 'x18'.
texinfo formatting would be @code{x18}. (Hopefully we'll move rst soon!)
Got it.
+This generally means that any code that may run on the same thread as code
+compiled with ShadowCallStack must be compiled with the flag
+@option{-ffixed-x18}, otherwise functions compiled without
+@option{-ffixed-x18} may clobber x18 and break scs.
Maybe:
might clobber @code{x18} and so corrupt the shadow stack pointer.
Ok.
+
+And also because there is no runtime support, the code compiled with
+ShadowCallStack cannot use exception handling, @option{-fno-exceptions}
+also needs to be specified.
Maybe:
Also, because there is no userspace runtime support, code compiled with
ShadowCallStack cannot use exception handling. Use @option{-fno-exceptions}
to turn off exceptions.
Ok.
index 55273822ec5..98e1d636f7f 100644
--- a/gcc/opts-global.c
+++ b/gcc/opts-global.c
@@ -477,4 +477,10 @@ handle_common_deferred_options (void)
gcc_unreachable ();
}
}
+
+#ifdef TARGET_CHECK_SCS_RESERVED_REGISTER
+ if (flag_sanitize & SANITIZE_SHADOW_CALL_STACK)
+ TARGET_CHECK_SCS_RESERVED_REGISTER ();
+#endif
+
Actually (after saying this should move from being a macro to a hook):
maybe we can keep it in target-specific code instead. We already have
similar errors in aarch64_override_options_internal.
(That comment only applies to this macro/hook. We still want a hook for
TARGET_SUPPORT_SHADOW_CALL_STACK.)
Ok, I'll put this check into aarch64_override_options_internal
and add a hook in gcc_target for TARGET_SUPPORT_SHADOW_CALL_STACK.
+ if (opts->x_flag_sanitize & SANITIZE_SHADOW_CALL_STACK)
+ {
+ if (!TARGET_SUPPORT_SHADOW_CALL_STACK)
+ error_at (loc, "%<-fsanitize=shadow-call-stack%> not supported "
+ "in current platform");
I think this should be a sorry() instead. It changes the diagnostic
prefix to “sorry, not implemented”, so that the compiler admits that
this is a missing feature rather than the user doing something wrong.
Thanks for the explanation, it sounds reasonable.
+
+ if (opts->x_flag_exceptions)
+ error_at (loc, "%<-fsanitize=shadow-call-stack%> only allowed "
+ "with %<-fno-exceptions%>");
Maybe better as an “else”, since reporting the second error seems
redundant after reporting the first error.
maybe: s/only allowed with/requires/
Arguably this could be a sorry() too, but it's not as clear. Let's stick
with error_at for now.
Got it!
Thanks,
Richard
+/* { dg-final { scan-assembler-times "str\tx30, \\\[x18\\\], #8" 2 } } */
+/* { dg-final { scan-assembler-times "ldr\tx30, \\\[x18, #-8\\\]!" 2 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/shadow_call_stack_4.c
b/gcc/testsuite/gcc.target/aarch64/shadow_call_stack_4.c
new file mode 100644
index 00000000000..a1b1cf16655
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/shadow_call_stack_4.c
@@ -0,0 +1,18 @@
+/* Testing the disable of shadow call stack. */
+/* scs_push: str x30, [x18], #8 */
+/* scs_pop: ldr x30, [x18, #-8]! */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fsanitize=shadow-call-stack -ffixed-x18 -fno-exceptions"
} */
+
+int foo (int);
+
+/* function disable shadow call stack. */
+int __attribute__((no_sanitize_shadow_call_stack)) func1 (void)
+{
+ asm volatile ("":::"x30");
+
+ return 0;
+}
+
+/* { dg-final { scan-assembler-not "str\tx30, \\\[x18\\\], #8" } } */
+/* { dg-final { scan-assembler-not "ldr\tx30, \\\[x18, #-8\\\]!" } } */
