diff mbox

RFC Asan instrumentation control

Message ID 52B95DB1.3090506@partner.samsung.com
State New
Headers show

Commit Message

max Dec. 24, 2013, 10:10 a.m. UTC
On 12/19/2013 04:27 PM, Jakub Jelinek wrote:
> On Thu, Dec 19, 2013 at 04:02:47PM +0400, Maxim Ostapenko wrote:
>> Sorry, ChangeLog and patch, of course.
>> 2013-12-19  Max Ostapenko  <m.ostapenko@partner.samsung.com>
>>
>> 	* cfgexpand.c (expand_stack_vars): Optionally disable asan stack protection.
> Too long lines in ChangeLog, wrap to 80 columns.

Thanks, fixed.

>> 	(expand_used_vars): Likewise.
>> 	(partition_stack_vars): Likewise.
>> 	* asan.c (asan_emit_stack_protection): Optionally disable after return stack usage.
> Ditto.

Likewise.

>> 	(instrument_derefs): Optionally disable memory access instrumentation.
>> 	(instrument_builtin_call): Likewise.
>> 	(instrument_strlen_call): Likewise.
>> 	(asan_protect_global): Optionally disable global variables protection.
>> 	* doc/invoke.texi: Added doc for new options.
>> 	* params.def: Added new options.
>> 	* params.h: Likewise.
>>
>> 2013-12-19  Max Ostapenko  <m.ostapenko@partner.samsung.com>
>> 	* c-c++-common/asan/global-overflow-2.c: New test.
> Missing vertical space between date/name/mail and first entry.

Done.

>> @@ -1003,7 +1004,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
>>     str_cst = asan_pp_string (&asan_pp);
>>   
>>     /* Emit the prologue sequence.  */
>> -  if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase)
>> +  if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase && ASAN_USE_AFTER_RETURN)
>>       {
>>         use_after_return_class = floor_log2 (asan_frame_size - 1) - 5;
>>         /* __asan_stack_malloc_N guarantees alignment
> Please wrap this.

Done.

>> --- a/gcc/cfgexpand.c
>> +++ b/gcc/cfgexpand.c
>> @@ -798,7 +798,7 @@ partition_stack_vars (void)
>>   	     sizes, as the shorter vars wouldn't be adequately protected.
>>   	     Don't do that for "large" (unsupported) alignment objects,
>>   	     those aren't protected anyway.  */
>> -	  if ((flag_sanitize & SANITIZE_ADDRESS) && isize != jsize
>> +	  if ((flag_sanitize & SANITIZE_ADDRESS) && ASAN_STACK  && isize != jsize
> Replace the two spaces with just one.

Done.

>> --- a/gcc/doc/invoke.texi
>> +++ b/gcc/doc/invoke.texi
>> @@ -10037,6 +10037,36 @@ 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 buffer overflow 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.
> Too long lines (several times).

Done.

>> +To disable memory reads instructions protection use @option{--param asan-instrument-reads=0} option.
>> +
>> +@item asan-instrument-writes
>> +Enable buffer overflow 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.
>> +
>> +@item asan-memintrin
>> +Enable detection for builtin functions. This kind of protection
> I think for docs it should be "built-in functions".

Done.

> As for the tests, I'm afraid I don't like them at all.
> If anything, it ought to be dg-do compile tests where you say
> scan assembly or some dump, but having runtime testcases that
> trigger undefined behavior that isn't detected by the instrumentation
> library at all and expect them to "pass" is simply wrong.
>
> 	Jakub
>
Got it, converted all tests except no-asan-stack.c, because i failed to 
discover how to grep for stack
instrumentation. Perhaps memcmp of random data is fine?

Comments

Jakub Jelinek Dec. 27, 2013, 10:39 a.m. UTC | #1
On Tue, Dec 24, 2013 at 02:10:57PM +0400, Maxim Ostapenko wrote:
> >As for the tests, I'm afraid I don't like them at all.
> >If anything, it ought to be dg-do compile tests where you say
> >scan assembly or some dump, but having runtime testcases that
> >trigger undefined behavior that isn't detected by the instrumentation
> >library at all and expect them to "pass" is simply wrong.
> >
> Got it, converted all tests except no-asan-stack.c, because i failed
> to discover how to grep for stack
> instrumentation. Perhaps memcmp of random data is fine?

It is still undefined behavior, IMNSHO it is far better to just change those
tests into compile time tests and verify assembly using dg-final
scan-assembler.  Say verify that __asan_option_detect_stack_use_after_return
isn't present for the no-use-after-return test, or __asan_report_store
resp. __asan_report_load isn't present, or __asan_register_globals
etc.

	Jakub
Yury Gribov Dec. 27, 2013, 11:06 a.m. UTC | #2
Jakub wrote:
>IMNSHO it is far better to just change those
> tests into compile time tests and verify assembly using dg-final
> scan-assembler.

We did exactly this for majority of tests except for
* c-c++-common/asan/use-after-return-1.c (back-ported from Clang, no UB)
* c-c++-common/asan/no-asan-stack.c (this triggers read overflow because 
we haven't found a cross-platform way to grep for stack redzones 
instrumentation)

We can simply remove no-asan-stack.c from the patch, then it won't 
contain any UB (but we won't have any test for --param no-asan-stack).

-Y
Jakub Jelinek Dec. 27, 2013, 11:12 a.m. UTC | #3
Hi!

On Fri, Dec 27, 2013 at 03:06:06PM +0400, Yury Gribov wrote:
> We did exactly this for majority of tests except for

Ah, sorry, I saw one test and didn't check carefully the rest of them.

> * c-c++-common/asan/use-after-return-1.c (back-ported from Clang, no UB)

Well, it is still UB, but one that should be detected by the sanitizer and
stopped before it happens, so it is ok.

> * c-c++-common/asan/no-asan-stack.c (this triggers read overflow
> because we haven't found a cross-platform way to grep for stack
> redzones instrumentation)

I'd prefer no test in that case, or just some semi-platform specific test
(scan that the 0x41b58ab3 constant doesn't appear in say some late RTL dump,
or perhaps just assembly (just scan it with lower and upper case and decimal
too)).

	Jakub
max Jan. 9, 2014, 8:11 a.m. UTC | #4
Hi!

 >> * c-c++-common/asan/no-asan-stack.c (this triggers read overflow
 >> because we haven't found a cross-platform way to grep for stack
 >> redzones instrumentation)

 > I'd prefer no test in that case, or just some semi-platform specific 
test
 > (scan that the 0x41b58ab3 constant doesn't appear in say some late 
RTL dump,
 > or perhaps just assembly (just scan it with lower and upper case and 
decimal
 > too)).

Thanks, commited in 206458 without c-c++-common/asan/no-asan-stack.c 
testfile.
I'll fix this test according to your recommendations a bit later.

-Maxim.
diff mbox

Patch

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

	* cfgexpand.c (expand_stack_vars): Optionally disable 
	asan stack protection.
	(expand_used_vars): Likewise.
	(partition_stack_vars): Likewise.
	* asan.c (asan_emit_stack_protection): Optionally disable 
	after return stack usage.
	(instrument_derefs): Optionally disable memory 
	access instrumentation.
	(instrument_builtin_call): Likewise.
	(instrument_strlen_call): Likewise.
	(asan_protect_global): Optionally disable 
	global variables protection.
	* doc/invoke.texi: Added doc for new options.
	* params.def: Added new options.
	* params.h: Likewise.

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

	* c-c++-common/asan/no-asan-globals.c: New test.
	* c-c++-common/asan/no-asan-stack.c: Likewise.
	* c-c++-common/asan/no-instrument-reads.c: Likewise.
	* c-c++-common/asan/no-instrument-writes.c: Likewise.
	* c-c++-common/asan/use-after-return-1.c: Likewise.
	* c-c++-common/asan/no-use-after-return.c: Likewise.


diff --git a/gcc/asan.c b/gcc/asan.c
index d4059d6..1d9d8ae 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.
@@ -1003,7 +1004,8 @@  asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
   str_cst = asan_pp_string (&asan_pp);
 
   /* Emit the prologue sequence.  */
-  if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase)
+  if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase
+      && ASAN_USE_AFTER_RETURN)
     {
       use_after_return_class = floor_log2 (asan_frame_size - 1) - 5;
       /* __asan_stack_malloc_N guarantees alignment
@@ -1239,6 +1241,9 @@  asan_needs_local_alias (tree decl)
 bool
 asan_protect_global (tree decl)
 {
+  if (!ASAN_GLOBALS)
+    return false;
+
   rtx rtl, symbol;
 
   if (TREE_CODE (decl) == STRING_CST)
@@ -1568,6 +1573,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;
 
@@ -1897,6 +1907,9 @@  instrument_strlen_call (gimple_stmt_iterator *iter)
 static bool
 instrument_builtin_call (gimple_stmt_iterator *iter)
 {
+  if (!ASAN_MEMINTRIN)
+    return false;
+
   bool iter_advanced_p = false;
   gimple call = gsi_stmt (*iter);
 
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 7a93975..21978ca 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -798,7 +798,7 @@  partition_stack_vars (void)
 	     sizes, as the shorter vars wouldn't be adequately protected.
 	     Don't do that for "large" (unsupported) alignment objects,
 	     those aren't protected anyway.  */
-	  if ((flag_sanitize & SANITIZE_ADDRESS) && isize != jsize
+	  if ((flag_sanitize & SANITIZE_ADDRESS) && ASAN_STACK && isize != jsize
 	      && ialign * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT)
 	    break;
 
@@ -981,7 +981,7 @@  expand_stack_vars (bool (*pred) (size_t), struct stack_vars_data *data)
       if (alignb * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT)
 	{
 	  base = virtual_stack_vars_rtx;
-	  if ((flag_sanitize & SANITIZE_ADDRESS) && pred)
+	  if ((flag_sanitize & SANITIZE_ADDRESS) && ASAN_STACK && pred)
 	    {
 	      HOST_WIDE_INT prev_offset = frame_offset;
 	      tree repr_decl = NULL_TREE;
@@ -1160,7 +1160,7 @@  defer_stack_allocation (tree var, bool toplevel)
   /* If stack protection is enabled, *all* stack variables must be deferred,
      so that we can re-order the strings to the top of the frame.
      Similarly for Address Sanitizer.  */
-  if (flag_stack_protect || (flag_sanitize & SANITIZE_ADDRESS))
+  if (flag_stack_protect || ((flag_sanitize & SANITIZE_ADDRESS) && ASAN_STACK))
     return true;
 
   /* We handle "large" alignment via dynamic allocation.  We want to handle
@@ -1820,7 +1820,7 @@  expand_used_vars (void)
 	    expand_stack_vars (stack_protect_decl_phase_2, &data);
 	}
 
-      if (flag_sanitize & SANITIZE_ADDRESS)
+      if ((flag_sanitize & SANITIZE_ADDRESS) && ASAN_STACK)
 	/* Phase 3, any partitions that need asan protection
 	   in addition to phase 1 and 2.  */
 	expand_stack_vars (asan_decl_phase_3, &data);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 60675c5..79ffad7 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10050,6 +10050,41 @@  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 buffer overflow 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}.
+
+@item asan-stack
+Enable buffer overflow detection for stack objects.  This kind of
+protection is enabled by default when using@option{-fsanitize=address}.
+To disable stack protection use @option{--param asan-stack=0} option.
+
+@item asan-instrument-reads
+Enable buffer overflow detection for memory reads.  This kind of
+protection is enabled by default when using @option{-fsanitize=address}.
+To disable memory reads protection use
+@option{--param asan-instrument-reads=0}.
+
+@item asan-instrument-writes
+Enable buffer overflow detection for memory writes.  This kind of
+protection is enabled by default when using @option{-fsanitize=address}.
+To disable memory writes protection use
+@option{--param asan-instrument-writes=0} option.
+
+@item asan-memintrin
+Enable detection for built-in functions.  This kind of protection
+is enabled by default when using @option{-fsanitize=address}.
+To disable built-in functions protection use
+@option{--param asan-memintrin=0}.
+
+@item asan-use-after-return
+Enable detection of use-after-return.  This kind of protection
+is enabled by default when using @option{-fsanitize=address} option.
+To disable use-after-return detection use 
+@option{--param asan-use-after-return=0}.
+
 @end table
 @end table
 
diff --git a/gcc/params.def b/gcc/params.def
index c0f9622..628fddc 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1049,7 +1049,37 @@  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)
+
+DEFPARAM (PARAM_ASAN_MEMINTRIN,
+         "asan-memintrin",
+         "Enable asan builtin functions protection",
+         1, 0, 1)
+
+DEFPARAM (PARAM_ASAN_USE_AFTER_RETURN,
+         "asan-use-after-return",
+         "Enable asan builtin functions protection",
+         1, 0, 1)
 /*
+
 Local variables:
 mode:c
 End:
diff --git a/gcc/params.h b/gcc/params.h
index f137e9e..49c1a2d 100644
--- a/gcc/params.h
+++ b/gcc/params.h
@@ -218,5 +218,17 @@  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)
+#define ASAN_MEMINTRIN \
+  PARAM_VALUE (PARAM_ASAN_MEMINTRIN)
+#define ASAN_USE_AFTER_RETURN \
+  PARAM_VALUE (PARAM_ASAN_USE_AFTER_RETURN)
 
 #endif /* ! GCC_PARAMS_H */
diff --git a/gcc/testsuite/c-c++-common/asan/no-asan-globals.c b/gcc/testsuite/c-c++-common/asan/no-asan-globals.c
new file mode 100644
index 0000000..70a1f95
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-asan-globals.c
@@ -0,0 +1,13 @@ 
+/* { dg-do assemble } */
+/* { dg-options "-save-temps --param asan-globals=0" } */
+
+volatile int ten = 10;
+
+int main() {
+  volatile static char XXX[10];
+  XXX[ten];
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not "__asan_register_globals" } } */
+/* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-asan-stack.c b/gcc/testsuite/c-c++-common/asan/no-asan-stack.c
new file mode 100644
index 0000000..636cacf
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-asan-stack.c
@@ -0,0 +1,14 @@ 
+/* { dg-do run } */
+/* { dg-options "--param asan-stack=0" } */
+#include <string.h>
+
+volatile int one = 1;
+
+int
+main ()
+{
+  volatile char a1[] = {one, 2, 3, 4};
+  volatile char a2[] = {1, 2*one, 3, 4};
+  volatile int res = memcmp ((void *)a1,(void *)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..df75878
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-instrument-reads.c
@@ -0,0 +1,13 @@ 
+/* { dg-do assemble } */
+/* { dg-options "--param asan-instrument-reads=0 -save-temps" } */
+
+volatile int ten = 10;
+
+int main() {
+  volatile char x[10];
+  x[ten];
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not "__asan_load" } } */
+/* { dg-final { cleanup-saved-temps } } */
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..c1500b9
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-instrument-writes.c
@@ -0,0 +1,13 @@ 
+/* { dg-do assemble } */
+/* { dg-options "--param asan-instrument-writes=0 -save-temps" } */
+
+volatile int ten = 10;
+
+int main() {
+  volatile char x[10];
+  x[ten] = 1;
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not "__asan_store" } } */
+/* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-use-after-return.c b/gcc/testsuite/c-c++-common/asan/no-use-after-return.c
new file mode 100644
index 0000000..f326e0c
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-use-after-return.c
@@ -0,0 +1,13 @@ 
+/* { dg-do assemble } */
+/* { dg-options "--param asan-use-after-return=0 -save-temps" } */
+
+extern void f(char *);
+
+int main() {
+  char buf[64];
+  f(buf);
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not "__asan_option_detect_stack_use_after_return" } } */
+/* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/c-c++-common/asan/use-after-return-1.c b/gcc/testsuite/c-c++-common/asan/use-after-return-1.c
new file mode 100644
index 0000000..435637d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/use-after-return-1.c
@@ -0,0 +1,53 @@ 
+/* { dg-do run } */
+/* { dg-set-target-env-var ASAN_OPTIONS "detect_stack_use_after_return=1" } */
+/* { dg-shouldfail "asan" } */
+
+#include <stdio.h>
+#include <pthread.h>
+
+#ifndef kSize
+# define kSize 1
+#endif
+
+#ifndef UseThread
+# define UseThread 0
+#endif
+
+__attribute__((noinline))
+char *Ident(char *x) {
+  fprintf(stderr, "1: %p\n", x);
+  return x;
+}
+
+__attribute__((noinline))
+char *Func1() {
+  char local[kSize];
+  return Ident(local);
+}
+
+__attribute__((noinline))
+void Func2(char *x) {
+  fprintf(stderr, "2: %p\n", x);
+  *x = 1;
+}
+
+void *Thread(void *unused)  {
+  Func2(Func1());
+  return NULL;
+}
+
+int main(int argc, char **argv) {
+#if UseThread
+  pthread_t t;
+  pthread_create(&t, 0, Thread, 0);
+  pthread_join(t, 0);
+#else
+  Func2(Func1());
+#endif
+  return 0;
+}
+
+/* { dg-output "WRITE of size 1 at .* thread T0.*" } */
+/* { dg-output "    #0.*Func2.*use-after-return-1.c:31.*" } */
+/* { dg-output "is located in stack of thread T0 at offset.*" } */
+/* { dg-output "\'local\' <== Memory access at offset 32 is inside this variable" } */