diff mbox

RFC Asan instrumentation control

Message ID 52B18887.9080605@partner.samsung.com
State New
Headers show

Commit Message

max Dec. 18, 2013, 11:35 a.m. UTC
Hi all,

On 12/06/2013 05:32 PM, Yury Gribov wrote:

> So it looks like people are generally ok with
> * --param asan-instrument-reads=0/1
> * --param asan-instrument-writes=0/1
> * --param asan-stack=0/1
> * --param asan-globals=0/1
I've implemented these options. Tested on x86_64.
> * --param asan-memintrin=0/1
> but not with blacklists (which is sad but understandable).
>
> -Y
>
This one will be implemented in future.

I've also added 4 new testfiles to test new options.

Can you review this patch, please?

-Maxim

Comments

Jakub Jelinek Dec. 18, 2013, 11:59 a.m. UTC | #1
On Wed, Dec 18, 2013 at 03:35:35PM +0400, Maxim Ostapenko wrote:
> 2013-12-18  Max Ostapenko  <m.ostapenko@partner.samsung.com>
> 
> 	* gcc/asan.c (asan_emit_stack_protection): Optionally disable stack protection.
> 	(instrument_derefs): Optionally disable memory access instrumentation.
> 	(instrument_mem_region_access): Likewise.
> 	(instrument_strlen_call): Likewise.
> 	(asan_finish_file): Optionally disable global variables protection.
> 	* gcc/doc/invoke.texi: Added doc for new options.
> 	* gcc/params.def: Added new options.
> 	* gcc/params.h: Likewise.

No gcc/ prefixes in ChangeLog entries.

> 2013-12-18  Max Ostapenko  <m.ostapenko@partner.samsung.com>
> 	* c-c++-common/asan/global-overflow-2.c: New test.
> 	* c-c++-common/asan/memcmp-3.c: Likewise.
> 	* c-c++-common/asan/no-instrument-reads.c: Likewise.
> 	* c-c++-common/asan/no-instrument-writes.c: Likewise.
> 
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -53,6 +53,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "gimple-builder.h"
>  #include "ubsan.h"
>  #include "predict.h"
> +#include "params.h"
>  
>  /* AddressSanitizer finds out-of-bounds and use-after-free bugs
>     with <2x slowdown on average.
> @@ -963,6 +964,9 @@ rtx
>  asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
>  			    HOST_WIDE_INT *offsets, tree *decls, int length)
>  {
> +  if (!ASAN_STACK)
> +    return NULL_RTX;

This is a wrong spot to do this.  Instead put it into the
if ((flag_sanitize & SANITIZE_ADDRESS) && pred)
condition in cfgexpand.c (and maybe also 
if ((flag_sanitize & SANITIZE_ADDRESS) && isize != jsize ...)
too, maybe all four flag_sanitize & SANITIZE_ADDRESS occurrences in
cfgexpand.c.

> @@ -2396,7 +2413,7 @@ asan_finish_file (void)
>        ++gcount;
>    htab_t const_desc_htab = constant_pool_htab ();
>    htab_traverse (const_desc_htab, count_string_csts, &gcount);
> -  if (gcount)
> +  if (gcount && ASAN_GLOBALS)
>      {
>        tree type = asan_global_struct (), var, ctor;
>        tree dtor_statements = NULL_TREE;

I'd say this isn't sufficient, for !ASAN_GLOBALS you should also make sure
asan_protect_global always returns false, so that no extra padding is emitted
around the global vars.

> +@item asan-stack
> +Enable overflow/underflow detection for stack objects. This kind of protection
> +is enabled by default if you are using @option{-fsanitize=address} option.
> +To disable stack protection use @option{--param asan-stack=0} option.

Talking about this, perhaps there should be also
--param asan-use-after-return=0
knob to disallow the support for use-after-return checking (in 4.8 this
didn't exist, in 4.9 there is some extra runtime code emitted, but still one
needs to enable it manually through environment variable).  With that param
we would emit pretty much what 4.8 did, i.e. assume that use-after-return
will not be enabled in the runtime.

	Jakub
Yury Gribov Dec. 19, 2013, 10:28 a.m. UTC | #2
> No gcc/ prefixes in ChangeLog entries.

Should we update contrib/mklog then?

-Y
Jakub Jelinek Dec. 19, 2013, 10:54 a.m. UTC | #3
On Thu, Dec 19, 2013 at 02:28:11PM +0400, Yury Gribov wrote:
> > No gcc/ prefixes in ChangeLog entries.
> 
> Should we update contrib/mklog then?

In my experience mklog is pretty much useless, e.g. if you
add a new function, it will list the previous function as being modified
rather than the new one, etc.

	Jakub
max Dec. 19, 2013, 11:59 a.m. UTC | #4
> 2013-12-18 Max Ostapenko<m.ostapenko@partner.samsung.com>
>
>     * gcc/asan.c (asan_emit_stack_protection): Optionally disable 
> stack protection.
>     (instrument_derefs): Optionally disable memory access 
> instrumentation.
>     (instrument_mem_region_access): Likewise.
>     (instrument_strlen_call): Likewise.
>     (asan_finish_file): Optionally disable global variables protection.
>     * gcc/doc/invoke.texi: Added doc for new options.
>     * gcc/params.def: Added new options.
>     * gcc/params.h: Likewise.
 > No gcc/ prefixes in ChangeLog entries.

Thanks, fixed.
> --- a/gcc/asan.c
> +++ b/gcc/asan.c
> @@ -53,6 +53,7 @@ along with GCC; see the file COPYING3.  If not see
>   #include "gimple-builder.h"
>   #include "ubsan.h"
>   #include "predict.h"
> +#include "params.h"
>     /* AddressSanitizer finds out-of-bounds and use-after-free bugs
>      with <2x slowdown on average.
> @@ -963,6 +964,9 @@ rtx
>   asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
>                   HOST_WIDE_INT *offsets, tree *decls, int length)
>   {
> +  if (!ASAN_STACK)
> +    return NULL_RTX;
 > This is a wrong spot to do this.  Instead put it into the
 > if ((flag_sanitize & SANITIZE_ADDRESS) && pred)
 > condition in cfgexpand.c (and maybe also
 > if ((flag_sanitize & SANITIZE_ADDRESS) && isize != jsize ...)
 > too, maybe all four flag_sanitize & SANITIZE_ADDRESS occurrences in
 > cfgexpand.c.

Moved this check to cfgexpand.c.

> @@ -2396,7 +2413,7 @@ asan_finish_file (void)
>         ++gcount;
>     htab_t const_desc_htab = constant_pool_htab ();
>     htab_traverse (const_desc_htab, count_string_csts, &gcount);
> -  if (gcount)
> +  if (gcount && ASAN_GLOBALS)
>       {
>         tree type = asan_global_struct (), var, ctor;
>         tree dtor_statements = NULL_TREE;

 > I'd say this isn't sufficient, for !ASAN_GLOBALS you should also make 
sure
 > asan_protect_global always returns false, so that no extra padding is 
emitted
 > around the global vars.

Moved globals protection check to asan_protect_global.

 > Talking about this, perhaps there should be also
 > --param asan-use-after-return=0
 > knob to disallow the support for use-after-return checking (in 4.8 this
 > didn't exist, in 4.9 there is some extra runtime code emitted, but 
still one
 > needs to enable it manually through environment variable). With that 
param
 > we would emit pretty much what 4.8 did, i.e. assume that 
use-after-return
 > will not be enabled in the runtime.


Added this option and also implemented asan-memintrin option.
Is it OK?

-Maxim
diff mbox

Patch

2013-12-18  Max Ostapenko  <m.ostapenko@partner.samsung.com>

	* gcc/asan.c (asan_emit_stack_protection): Optionally disable stack protection.
	(instrument_derefs): Optionally disable memory access instrumentation.
	(instrument_mem_region_access): Likewise.
	(instrument_strlen_call): Likewise.
	(asan_finish_file): Optionally disable global variables protection.
	* gcc/doc/invoke.texi: Added doc for new options.
	* gcc/params.def: Added new options.
	* gcc/params.h: Likewise.

2013-12-18  Max Ostapenko  <m.ostapenko@partner.samsung.com>
	* c-c++-common/asan/global-overflow-2.c: New test.
	* c-c++-common/asan/memcmp-3.c: Likewise.
	* c-c++-common/asan/no-instrument-reads.c: Likewise.
	* c-c++-common/asan/no-instrument-writes.c: Likewise.

diff --git a/gcc/asan.c b/gcc/asan.c
index 1394e13..1b8d0c2 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -53,6 +53,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "gimple-builder.h"
 #include "ubsan.h"
 #include "predict.h"
+#include "params.h"
 
 /* AddressSanitizer finds out-of-bounds and use-after-free bugs
    with <2x slowdown on average.
@@ -963,6 +964,9 @@  rtx
 asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
 			    HOST_WIDE_INT *offsets, tree *decls, int length)
 {
+  if (!ASAN_STACK)
+    return NULL_RTX;
+
   rtx shadow_base, shadow_mem, ret, mem, orig_base, lab;
   char buf[30];
   unsigned char shadow_bytes[4];
@@ -1568,6 +1572,11 @@  static void
 instrument_derefs (gimple_stmt_iterator *iter, tree t,
 		   location_t location, bool is_store)
 {
+  if (is_store && !ASAN_INSTRUMENT_WRITES)
+    return;
+  if (!is_store && !ASAN_INSTRUMENT_READS)
+    return;
+
   tree type, base;
   HOST_WIDE_INT size_in_bytes;
 
@@ -1662,6 +1671,11 @@  instrument_mem_region_access (tree base, tree len,
 			      gimple_stmt_iterator *iter,
 			      location_t location, bool is_store)
 {
+  if (is_store && !ASAN_INSTRUMENT_WRITES)
+    return;
+  if (!is_store && !ASAN_INSTRUMENT_READS)
+    return;
+
   if (!POINTER_TYPE_P (TREE_TYPE (base))
       || !INTEGRAL_TYPE_P (TREE_TYPE (len))
       || integer_zerop (len))
@@ -1825,6 +1839,9 @@  instrument_mem_region_access (tree base, tree len,
 static bool
 instrument_strlen_call (gimple_stmt_iterator *iter)
 {
+  if (!ASAN_INSTRUMENT_READS)
+    return false;
+
   gimple call = gsi_stmt (*iter);
   gcc_assert (is_gimple_call (call));
 
@@ -2396,7 +2413,7 @@  asan_finish_file (void)
       ++gcount;
   htab_t const_desc_htab = constant_pool_htab ();
   htab_traverse (const_desc_htab, count_string_csts, &gcount);
-  if (gcount)
+  if (gcount && ASAN_GLOBALS)
     {
       tree type = asan_global_struct (), var, ctor;
       tree dtor_statements = NULL_TREE;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 99ec1d2..d1f20a9 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10037,6 +10037,26 @@  The default choice depends on the target.
 Set the maximum number of existing candidates that will be considered when
 seeking a basis for a new straight-line strength reduction candidate.
 
+@item asan-globals
+Enable overflow/underflow detection for global objects. This kind of protection
+is enabled by default if you are using @option{-fsanitize=address} option.
+To disable global objects protection use @option{--param asan-globals=0} option.
+
+@item asan-stack
+Enable overflow/underflow detection for stack objects. This kind of protection
+is enabled by default if you are using @option{-fsanitize=address} option.
+To disable stack protection use @option{--param asan-stack=0} option.
+
+@item asan-instrument-reads
+Enable overflow/underflow detection for memory reads instructions. This kind of protection
+is enabled by default if you are using @option{-fsanitize=address} option.
+To disable memory reads instructions protection use @option{--param asan-instrument-reads=0} option.
+
+@item asan-instrument-writes
+Enable overflow/underflow detection for memory writes instructions. This kind of protection
+is enabled by default if you are using @option{-fsanitize=address} option.
+To disable memory writes instructions protection use @option{--param asan-instrument-writes=0} option.
+
 @end table
 @end table
 
diff --git a/gcc/params.def b/gcc/params.def
index c0f9622..aea5f41 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1049,6 +1049,26 @@  DEFPARAM (PARAM_MAX_SLSR_CANDIDATE_SCAN,
 	  "strength reduction",
 	  50, 1, 999999)
 
+DEFPARAM(PARAM_ASAN_STACK,
+         "asan-stack",
+         "Enable asan stack protection",
+         1, 0, 1)
+
+DEFPARAM(PARAM_ASAN_GLOBALS,
+         "asan-globals",
+         "Enable asan globals protection",
+         1, 0, 1)
+
+DEFPARAM(PARAM_ASAN_INSTRUMENT_WRITES,
+         "asan-instrument-writes",
+         "Enable asan store operations protection",
+         1, 0, 1)
+
+DEFPARAM(PARAM_ASAN_INSTRUMENT_READS,
+         "asan-instrument-reads",
+         "Enable asan load operations protection",
+         1, 0, 1)
+
 /*
 Local variables:
 mode:c
diff --git a/gcc/params.h b/gcc/params.h
index f137e9e..bbc2929 100644
--- a/gcc/params.h
+++ b/gcc/params.h
@@ -218,5 +218,14 @@  extern void init_param_values (int *params);
   PARAM_VALUE (PARAM_ALLOW_PACKED_LOAD_DATA_RACES)
 #define ALLOW_PACKED_STORE_DATA_RACES \
   PARAM_VALUE (PARAM_ALLOW_PACKED_STORE_DATA_RACES)
+#define ASAN_STACK \
+  PARAM_VALUE (PARAM_ASAN_STACK)
+#define ASAN_GLOBALS \
+  PARAM_VALUE (PARAM_ASAN_GLOBALS)
+#define ASAN_INSTRUMENT_READS \
+  PARAM_VALUE (PARAM_ASAN_INSTRUMENT_READS)
+#define ASAN_INSTRUMENT_WRITES \
+  PARAM_VALUE (PARAM_ASAN_INSTRUMENT_WRITES)
+
 
 #endif /* ! GCC_PARAMS_H */
diff --git a/gcc/testsuite/c-c++-common/asan/global-overflow-2.c b/gcc/testsuite/c-c++-common/asan/global-overflow-2.c
new file mode 100644
index 0000000..5fe7ba3
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/global-overflow-2.c
@@ -0,0 +1,22 @@ 
+/* { dg-do run } */
+/* { dg-options "--param asan-globals=0" } */
+#include <stdio.h>
+extern
+#ifdef __cplusplus
+"C"
+#endif
+void *memset (void *, int, __SIZE_TYPE__);
+
+volatile int ten = 10;
+
+int main() {
+  static char XXX[10];
+  static char YYY[10];
+  static char ZZZ[10];
+  memset(XXX, 0, 10);
+  memset(YYY, 0, 10);
+  memset(ZZZ, 0, 10);
+  int res = YYY[ten];  /* BOOOM */
+  res += XXX[ten/10] + ZZZ[ten/10];
+  return 0;
+}
diff --git a/gcc/testsuite/c-c++-common/asan/memcmp-3.c b/gcc/testsuite/c-c++-common/asan/memcmp-3.c
new file mode 100644
index 0000000..2f50939
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/memcmp-3.c
@@ -0,0 +1,14 @@ 
+/* { dg-do run } */
+/* { dg-options "--param asan-stack=0" } */
+#include <string.h>
+
+volatile int one = 1;
+
+int
+main ()
+{
+  char a1[] = {one, 2, 3, 4};
+  char a2[] = {1, 2*one, 3, 4};
+  int res = memcmp (a1, a2, 5 + one);
+  return 0;
+}
diff --git a/gcc/testsuite/c-c++-common/asan/no-instrument-reads.c b/gcc/testsuite/c-c++-common/asan/no-instrument-reads.c
new file mode 100644
index 0000000..6098545
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-instrument-reads.c
@@ -0,0 +1,18 @@ 
+/* { dg-do run } */
+/* { dg-options "--param asan-instrument-reads=0" } */
+
+extern
+#ifdef __cplusplus
+"C"
+#endif
+void *memset (void *, int, __SIZE_TYPE__);
+
+volatile int ten = 10;
+
+int main() {
+  volatile char x[10];
+  memset((void*) x, 0, 10);
+  volatile int a = x[ten];  /* BOOOM */
+  return 0;
+}
+
diff --git a/gcc/testsuite/c-c++-common/asan/no-instrument-writes.c b/gcc/testsuite/c-c++-common/asan/no-instrument-writes.c
new file mode 100644
index 0000000..2569f39
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-instrument-writes.c
@@ -0,0 +1,19 @@ 
+/* { dg-do run } */
+/* { dg-options "--param asan-instrument-writes=0" } */
+
+extern
+#ifdef __cplusplus
+"C"
+#endif
+void *memset (void *, int, __SIZE_TYPE__);
+
+volatile int ten = 10;
+
+int main() {
+  volatile char x[10];
+  volatile int y;
+  memset((void *)x, 0, 10);
+  x[ten] = 1;  /* BOOOM */
+  return 0;
+}
+