[PING^3][PATCH,v2,1/1,AARCH64][PR102768] aarch64: Add compiler support for Shadow Call Stack

[Dan Li]

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\\\]!" } } */