Re: [PATCH 2/3][GCC][AARCH64] Add new -mbranch-protection option to combine pointer signing and BTI

[Sudakshina Das <Sudi.Das@arm.com>]

Hi Sam

On 02/11/18 17:31, Sam Tebbs wrote:
> Hi all,
> 
> The -mbranch-protection option combines the functionality of
> -msign-return-address and the BTI features new in Armv8.5 to better reflect
> their relationship. This new option therefore supersedes and deprecates the
> existing -msign-return-address option.
> 
> -mbranch-protection=[none|standard|<types>] - Turns on different types of branch
> protection available where:
> 
>       * "none": Turn of all types of branch protection
>       * "standard" : Turns on all the types of protection to their respective
>         standard levels.
>       * <types> can be "+" separated protection types:
> 
>       	* "bti" : Branch Target Identification Mechanism.
> 	* "pac-ret{+leaf+b-key}": Return Address Signing. The default return
> 	  address signing is enabled by signing functions that save the return
> 	  address to memory (non-leaf functions will practically always do this)
> 	  using the a-key. The optional tuning arguments allow the user to
> 	  extend the scope of return address signing to include leaf functions
> 	  and to change the key to b-key. The tuning arguments must proceed the
> 	  protection type "pac-ret".
> 
> Thus -mbranch-protection=standard -> -mbranch-protection=bti+pac-ret.
> 
> Its mapping to -msign-return-address is as follows:
> 
>       * -mbranch-protection=none -> -msign-return-address=none
>       * -mbranch-protection=standard -> -msign-return-address=leaf
>       * -mbranch-protection=pac-ret -> -msign-return-address=non-leaf
>       * -mbranch-protection=pac-ret+leaf -> -msign-return-address=all
> 
> This patch implements the option's skeleton and the "none", "standard" and
> "pac-ret" types (along with its "leaf" subtype).
> 
> The previous patch in this series is here:
> https://gcc.gnu.org/ml/gcc-patches/2018-11/msg00103.html
> 
> Bootstrapped successfully and tested on aarch64-none-elf with no regressions.
> 
> OK for trunk?
> 

Thank for doing this. I am not a maintainer so you will need a
maintainer's approval. Only nit, that I would add is that it would
be good to have more test coverage, specially for the new parsing
functions that have been added and the errors that are added.

Example checking a few valid and invalid combinations of the options
like:
-mbranch-protection=pac-ret -mbranch-protection=none //disables
everything
-mbranch-protection=leaf  //errors out
-mbranch-protection=none+pac-ret //errors out
... etc

Also instead of removing all the old deprecated options, you can keep
one (or a copy of one) to check for the deprecated warning.


diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 
e290128f535f3e6b515bff5a81fae0aa0d1c8baf..07cfe69dc3dd9161a2dd93089ccf52ef251208d2 
100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -15221,13 +15222,18 @@ accessed using a single instruction and 
emitted after each function.  This
  limits the maximum size of functions to 1MB.  This is enabled by 
default for
  @option{-mcmodel=tiny}.

-@item -msign-return-address=@var{scope}
-@opindex msign-return-address
-Select the function scope on which return address signing will be applied.
-Permissible values are @samp{none}, which disables return address signing,
-@samp{non-leaf}, which enables pointer signing for functions which are 
not leaf
-functions, and @samp{all}, which enables pointer signing for all 
functions.  The
-default value is @samp{none}.
+@item 
-mbranch-protection=@var{none}|@var{standard}|@var{pac-ret}[+@var{leaf}]
+@opindex mbranch-protection
+Select the branch protection features to use.
+@samp{none} is the default and turns off all types of branch protection.
+@samp{standard} turns on all types of branch protection features.  If a 
feature
+has additional tuning options, then @samp{standard} sets it to its standard
+level.
+@samp{pac-ret[+@var{leaf}]} turns on return address signing to its standard
+level: signing functions that save the return address to memory (non-leaf
+functions will practically always do this) using the a-key.  The optional
+argument @samp{leaf} can be used to extend the signing to include leaf
+functions.

I am not sure if deleting the previous documentation of
-msign-retun-address is the way to go. Maybe add a "this has been
deprecated and refer to -mbranch-protection" to its description.

Thanks
Sudi

> gcc/ChangeLog:
> 
> 2018-11-02  Sam Tebbs<sam.tebbs@arm.com>
> 
> 	* config/aarch64/aarch64.c (BRANCH_PROTEC_STR_MAX,
> 	aarch64_parse_branch_protection,
> 	struct aarch64_branch_protec_type,
> 	aarch64_handle_no_branch_protection,
> 	aarch64_handle_standard_branch_protection,
> 	aarch64_validate_mbranch_protection,
> 	aarch64_handle_pac_ret_protection,
> 	aarch64_handle_attr_branch_protection,
> 	accepted_branch_protection_string,
> 	aarch64_pac_ret_subtypes,
> 	aarch64_branch_protec_types,
> 	aarch64_handle_pac_ret_leaf): Define.
> 	(aarch64_override_options_after_change_1): Add check for
> 	accepted_branch_protection_string.
> 	(aarch64_override_options): Add check for
> 	accepted_branch_protection_string.
> 	(aarch64_option_save): Save accepted_branch_protection_string.
> 	(aarch64_option_restore): Save
> 	accepted_branch_protection_string.
> 	* config/aarch64/aarch64.c (aarch64_attributes): Add branch-protection.
> 	* config/aarch64/aarch64.opt: Add mbranch-protection. Deprecate
> 	msign-return-address.
> 	* doc/invoke.texi: Add mbranch-protection.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-11-02  Sam Tebbs<sam.tebbs@arm.com>
> 
> 	* (gcc.target/aarch64/return_address_sign_1.c,
> 	gcc.target/aarch64/return_address_sign_2.c,
> 	gcc.target/aarch64/return_address_sign_3.c (__attribute__)): Change
> 	option to -mbranch-protection.
>