Matthew Malcomson <matthew.malcom...@arm.com> writes:
> ############### Attachment also inlined for ease of reply
> ###############
>
>
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index
> 372148315389db6671dfd943fd1a68670fcb1cbc..f8bf165aa48b5709c26f4e8245e5ab929b44fca6
> 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -54,6 +54,8 @@ static tree handle_cold_attribute (tree *, tree, tree, int,
> bool *);
> static tree handle_no_sanitize_attribute (tree *, tree, tree, int, bool *);
> static tree handle_no_sanitize_address_attribute (tree *, tree, tree,
> int, bool *);
> +static tree handle_no_sanitize_hwaddress_attribute (tree *, tree, tree,
> + int, bool *);
> static tree handle_no_sanitize_thread_attribute (tree *, tree, tree,
> int, bool *);
> static tree handle_no_address_safety_analysis_attribute (tree *, tree, tree,
> @@ -412,6 +414,8 @@ const struct attribute_spec c_common_attribute_table[] =
> handle_no_sanitize_attribute, NULL },
> { "no_sanitize_address", 0, 0, true, false, false, false,
> handle_no_sanitize_address_attribute, NULL },
> + { "no_sanitize_hwaddress", 0, 0, true, false, false, false,
> + handle_no_sanitize_hwaddress_attribute, NULL },
> { "no_sanitize_thread", 0, 0, true, false, false, false,
> handle_no_sanitize_thread_attribute, NULL },
> { "no_sanitize_undefined", 0, 0, true, false, false, false,
> @@ -946,6 +950,22 @@ handle_no_sanitize_address_attribute (tree *node, tree
> name, tree, int,
> return NULL_TREE;
> }
>
> +/* Handle a "no_sanitize_hwaddress" attribute; arguments as in
> + struct attribute_spec.handler. */
> +
> +static tree
> +handle_no_sanitize_hwaddress_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_HWADDRESS);
> +
> + return NULL_TREE;
> +}
> +
> /* Handle a "no_sanitize_thread" attribute; arguments as in
> struct attribute_spec.handler. */
>
Although the Clang design page mentions this attribute, it doesn't look
like it was ever committed upstream (unless I'm missing something).
Clang instead seems to require:
__attribute__((no_sanitize("hwaddress")))
__attribute__((no_sanitize("kernel-hwaddress")))
That seems more scalable than adding an extra attribute for each new
sanitiser, and of course is what this patch also supports.
So it might be better to drop this and just stick with what Clang provides.
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index
> ba18e05fb1abd0034afb73fd4a20feac27133149..97a5a532e31a9cea20955863bf6d2c8911a8e869
> 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -13898,13 +13898,34 @@ more details. The run-time behavior can be
> influenced using the
> the available options are shown at startup of the instrumented program. See
>
> @url{https://github.com/google/sanitizers/wiki/AddressSanitizerFlags#run-time-flags}
> for a list of supported options.
> -The option cannot be combined with @option{-fsanitize=thread}.
> +The option cannot be combined with @option{-fsanitize=thread} or
> +@option{-fsanitize=hwaddress}.
>
> @item -fsanitize=kernel-address
> @opindex fsanitize=kernel-address
> Enable AddressSanitizer for Linux kernel.
> See @uref{https://github.com/google/kasan/wiki} for more details.
>
> +@item -fsanitize=hwaddress
> +@opindex fsanitize=hwaddress
> +Enable HardwareAddressSanitizer, a fast memory error detector.
Is HardwareAddressSanitizer an established shorthand? All the references
I could see instead referred to it as “Hardware-assisted AddressSanitizer”,
which seems a bit more descriptive.
It would be good to expand on “a fast memory error detector“ a bit.
I was wondering about something like “, which uses dedicated features
of the target hardware to accelerate the detection of memory errors”.
> +Memory access instructions are instrumented to detect out-of-bounds and
> +use-after-free bugs.
> +The option enables @option{-fsanitize-address-use-after-scope}.
> +See
> +@uref{https://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html}
> +for more details. The run-time behavior can be influenced using the
> +@env{HWASAN_OPTIONS} environment variable. When set to @code{help=1},
> +the available options are shown at startup of the instrumented program.
> +The option cannot be combined with @option{-fsanitize=thread} or
> +@option{-fsanitize=address}.
I think it would be worth emphasising that this option is only supported
on AArch64.
> +
> +@item -fsanitize=kernel-hwaddress
> +@opindex fsanitize=kernel-hwaddress
> +Enable HardwareAddressSanitizer for Linux kernel.
IMO “for Linux” or “for the Linux kernel” reads better.
> +Similar to @option{-fsanitize=kernel-address} but using the software tagging
> +method of instrumentation.
“the software tagging method of instrumentation” makes it sound like
users are already expected to know what that is. I think this should
be expanded a bit.
> diff --git a/gcc/opts.c b/gcc/opts.c
> index
> 499eb9006431f178d243fe99d9b776a4b00ade90..ff0d48949dcf94a86de00c16087ef1b6381a2328
> 100644
> --- a/gcc/opts.c
> +++ b/gcc/opts.c
> @@ -1070,6 +1070,13 @@ finish_options (struct gcc_options *opts, struct
> gcc_options *opts_set,
> "%<-fsanitize=address%> or %<-fsanitize=kernel-address%>");
> }
>
> + /* Userspace and kernel HWasan conflict with each other. */
> + if ((opts->x_flag_sanitize & SANITIZE_USER_HWADDRESS)
> + && (opts->x_flag_sanitize & SANITIZE_KERNEL_HWADDRESS))
> + error_at (loc,
> + "%<-fsanitize=hwaddress%> is incompatible with "
> + "%<-fsanitize=kernel-hwaddress%>");
> +
> /* Userspace and kernel ASan conflict with each other. */
> if ((opts->x_flag_sanitize & SANITIZE_USER_ADDRESS)
> && (opts->x_flag_sanitize & SANITIZE_KERNEL_ADDRESS))
I realise this is making more work for you, sorry, but I think we
should avoid having so many errors of the same form but with different
option names hard-coded into the strings. Doing that just creates more
work for the translators. IMO this and existing errors should use:
error_at (loc, "%qs is incompatible with %qs", "-f…", "-f…");
> @@ -1089,6 +1096,20 @@ finish_options (struct gcc_options *opts, struct
> gcc_options *opts_set,
> error_at (loc,
> "%<-fsanitize=leak%> is incompatible with %<-fsanitize=thread%>");
>
> + /* HWASan and ASan conflict with each other. */
> + if ((opts->x_flag_sanitize & SANITIZE_ADDRESS)
> + && (opts->x_flag_sanitize & SANITIZE_HWADDRESS))
> + error_at (loc,
> + "%<-fsanitize=hwaddress%> is incompatible with both "
> + "%<-fsanitize=address%> and %<-fsanitize=kernel-address%>");
> +
> + /* HWASan conflicts with TSan. */
> + if ((opts->x_flag_sanitize & SANITIZE_HWADDRESS)
> + && (opts->x_flag_sanitize & SANITIZE_THREAD))
> + error_at (loc,
> + "%<-fsanitize=hwaddress%> is incompatible with "
> + "%<-fsanitize=thread%>");
It looks like these tests are detecting errors for
-fsanitize=kernel-hwaddress as well as -fsanitize=hwaddress.
This is again an existing problem, but it seems like bad UI to list
multiple options that the user might have specified when we know
exactly which ones they did specify. I.e. each error should only
need to specify two options.
Rather than trying to catch all the combinations individually, perhaps
it would be better to have a utility function along the lines of:
report_conflicting_sanitizer_options (flags1, flags2)
where FLAGS1 and FLAGS2 are bitmasks of SANITIZER_* options.
The function could then use sanitizer_opts to report an appropriate
error if one option from FLAGS1 is set at the same time as a different
option from FLAGS2, or vice versa.
> +
> @@ -2245,6 +2272,14 @@ common_handle_option (struct gcc_options *opts,
> SET_OPTION_IF_UNSET (opts, opts_set, param_asan_protect_allocas, 0);
> SET_OPTION_IF_UNSET (opts, opts_set, param_asan_use_after_return, 0);
> }
> + if (opts->x_flag_sanitize & SANITIZE_KERNEL_HWADDRESS)
> + {
> + SET_OPTION_IF_UNSET (opts, opts_set,
> + param_hwasan_instrument_stack, 0);
> + SET_OPTION_IF_UNSET (opts, opts_set,
> + param_hwasan_random_frame_tag, 0);
> + SET_OPTION_IF_UNSET (opts, opts_set, param_hwasan_protect_allocas, 0);
> + }
I think the documentation should make it clearer that
-fsanitize=kernel-hwaddress offers a different level of protection
from -fsanitize=hwaddress. It sounded from the documentation above
like it was a pure performance choice.
> diff --git a/gcc/params.opt b/gcc/params.opt
> index
> b36eee0e71f24a3201879b73c1cd0de3298cac32..c792670a4b88539ff52d64139cef5d44da469b32
> 100644
> --- a/gcc/params.opt
> +++ b/gcc/params.opt
> @@ -62,6 +62,30 @@ Enable asan stack protection.
> Common Joined UInteger Var(param_asan_use_after_return) Init(1)
> IntegerRange(0, 1) Param Optimization
> Enable asan detection of use-after-return bugs.
>
> +-param=hwasan-instrument-stack=
> +Common Joined UInteger Var(param_hwasan_instrument_stack) Init(1)
> IntegerRange(0, 1) Param Optimization
> +Enable hwasan stack protection.
I think it would be worth expanding this a bit. “stack protection”
can mean different things :-)
Also, “protection” seems like a dangerous word, especially given that we
continue after a problem has been detected. The sanitisers are more about
detecting errors and finding bugs rather than trying to protect against
an attacker. Same comment for the rest.
> +-param=hwasan-random-frame-tag=
> +Common Joined UInteger Var(param_hwasan_random_frame_tag) Init(1)
> IntegerRange(0, 1) Param Optimization
> +Use random base tag for each frame, as opposed to base always zero.
> +
> +-param=hwasan-instrument-allocas=
> +Common Joined UInteger Var(param_hwasan_protect_allocas) Init(1)
> IntegerRange(0, 1) Param Optimization
> +Enable hwasan allocas/VLAs protection.
> +
> +-param=hwasan-instrument-reads=
> +Common Joined UInteger Var(param_hwasan_instrument_reads) Init(1)
> IntegerRange(0, 1) Param Optimization
> +Enable hwasan load operations protection.
> +
> +-param=hwasan-instrument-writes=
> +Common Joined UInteger Var(param_hwasan_instrument_writes) Init(1)
> IntegerRange(0, 1) Param Optimization
> +Enable hwasan store operations protection.
> +
> +-param=hwasan-instrument-mem-intrinsics=
> +Common Joined UInteger Var(param_hwasan_instrument_mem_intrinsics) Init(1)
> IntegerRange(0, 1) Param Optimization
> +Enable hwasan builtin functions protection.
> +
> -param=avg-loop-niter=
> Common Joined UInteger Var(param_avg_loop_niter) Init(10) IntegerRange(1,
> 65536) Param Optimization
> Average number of iterations of a loop.
--params also need to be documented in invoke.texi, even though we don't
guarantee a stable interface for them.
> diff --git a/gcc/target.def b/gcc/target.def
> index
> c11cab8891f9bf3c4c9b4357d7b4b0442560bb41..43b491b027615523f0202fe1ded430cac0b4bef4
> 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -6830,6 +6830,17 @@ DEFHOOK
> HOOK_VECTOR_END (mode_switching)
>
> #undef HOOK_PREFIX
> +#define HOOK_PREFIX "TARGET_MEMTAG_"
> +HOOK_VECTOR (TARGET_MEMTAG_, memtag)
> +
> +DEFHOOK
> +(can_tag_addresses,
> + "True if backend architecture naturally supports ignoring the top byte of\
> + pointers. This feature means that @option{-fsanitize=hwaddress} can work.",
> + bool, (), default_memtag_can_tag_addresses)
I'm a bit wary of hard-coding the idea that the tag must be exactly
a byte in size. How easy would it be to parameterise that?
If it's easy to do then it's probably worth it. If it's not easy
to do then I think it's fair to leave it to whoever needs a different
size in future.
> diff --git a/gcc/toplev.c b/gcc/toplev.c
> index
> 95eea63380f60ceae4996cac5f974d8a24b20061..cfbfc30ccfac9bd3bd3067bdddcdec71159a64a2
> 100644
> --- a/gcc/toplev.c
> +++ b/gcc/toplev.c
> @@ -1843,6 +1843,16 @@ process_options (void)
> flag_sanitize &= ~SANITIZE_ADDRESS;
> }
>
> + /* HWAsan requires top byte ignore feature in the backend. */
> + if (flag_sanitize & SANITIZE_HWADDRESS
> + && ! targetm.memtag.can_tag_addresses ())
> + {
> + warning_at (UNKNOWN_LOCATION, 0,
> + "%<-fsanitize=hwaddress%> can not be implemented on "
> + "a backend that does not ignore the top byte of a pointer");
> + flag_sanitize &= ~SANITIZE_HWADDRESS;
> + }
I think this should just be "%qs is not supported for this target".
If other architectures add support for a similar feature in future then
existing GCCs will be unaware of that feature. The limitation will then
be in GCC rather than the target.
Thanks,
Richard
