[PATCH] Inline asm asan instrumentation

[PATCH] Inline asm asan instrumentation

[Marat Zakirov]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

Hi all,

Here's a patch for optional Asan instrumentation of inline assembly.

This version scans gimple for GIMPLE_ASMs and performs usual instrumentation
of arguments with memory constraints ("m", "o", etc.) with fixed size.

Instrumentation is turned off by default.

This was successfully bootstrapped and regtested on x64. I have also
instrumented and ran ffmpeg regression testsuite (it seems to have quite
some inline asm).

--Marat

[-- Attachment #2: InAsmAsan.diff --]
[-- Type: application/octet-stream, Size: 8805 bytes --]

gcc/ChangeLog:

2014-05-28  Marat Zakirov  <m.zakirov@samsung.com>

	* asan.c (struct asm_cb): Add struct for callback params.
	(maybe_instrument_asm_cb): New function.
	(maybe_instrument_asm): Likewise.
	(transform_statements): Call maybe_instrument_asm.
	* doc/invoke.texi (asan-instrument-asm): Describe new option.
	* gimple.h (is_gimple_asm): New function.
	* params.def, params.h (ASAN_INSTRUMENT_ASM): New parameter.

gcc/testsuite/ChangeLog:

2014-05-28  Marat Zakirov  <m.zakirov@samsung.com>

	* c-c++-common/asan/asm-scalar-1.c: New test.
	* c-c++-common/asan/asm-scalar-2.c: New test.
	* c-c++-common/asan/asm-scalar-3.c: New test.
	* c-c++-common/asan/asm-struct-1.c: New test.
	* c-c++-common/asan/asm-struct-2.c: New test.


diff --git a/gcc/asan.c b/gcc/asan.c
index 118f9fc..403fb98 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -54,6 +54,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "ubsan.h"
 #include "predict.h"
 #include "params.h"
+#include "stmt.h"
+#include "gimple-walk.h"
 
 /* AddressSanitizer finds out-of-bounds and use-after-free bugs
    with <2x slowdown on average.
@@ -2059,6 +2061,77 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
   return false;
 }
 
+struct asm_cb
+{
+  gimple_stmt_iterator *iter;
+  int arg;
+  int noutputs;
+};
+
+/* Callback function for assembler support called via walk_tree.
+
+   This fuction handle inline assembler arguments and if need put
+   memory checks.  */
+
+static tree
+maybe_instrument_asm_cb (tree *tp, int *walk_subtrees, void *data)
+{
+  struct walk_stmt_info *wi = (struct walk_stmt_info *) data;
+  struct asm_cb * inp_p = (struct asm_cb *)wi->info;
+  location_t loc = gimple_location (gsi_stmt (*(inp_p->iter)));
+  HOST_WIDE_INT size_in_bytes;
+  bool is_store;
+
+  size_in_bytes = int_size_in_bytes (TREE_TYPE (*tp));
+
+  *walk_subtrees = 0;
+  if (wi->val_only || size_in_bytes == -1)
+   return NULL_TREE;
+
+  is_store = (inp_p->arg++ < inp_p->noutputs);
+
+  if (size_in_bytes <= 16)
+    {
+      instrument_derefs (inp_p->iter, *tp, loc, is_store);
+    }
+  else if (size_in_bytes < 0xffffffff)
+    {
+      tree len = build_int_cst (size_type_node, size_in_bytes);
+      tree base = build_fold_addr_expr (*tp);
+      instrument_mem_region_access (base, len, inp_p->iter, loc, is_store);
+    }
+  wi->changed = true;
+  return NULL_TREE;
+}
+
+/* Instrumentation of the inline assembly if it is subject to instrumentation.
+   At the moment the only memory constraints are instrumented regardless
+   to work logic of inline assebly itself.
+
+   Upon completion return TRUE iff *ITER was advanced to the statement
+   following the one it was originally pointing to.
+
+   Uses callback function maybe_instrument_asm_cb.  */
+
+static bool
+maybe_instrument_asm (gimple_stmt_iterator *iter)
+{
+  if (!ASAN_INSTRUMENT_ASM)
+     return false;
+  struct walk_stmt_info wi;
+  gimple stmt = gsi_stmt (*iter);
+  struct asm_cb inp;
+  inp.iter = iter;
+  inp.arg = 0;
+  inp.noutputs = gimple_asm_noutputs (stmt);
+  wi.info = (void *)&inp;
+  walk_gimple_op (stmt, maybe_instrument_asm_cb, &wi);
+  if (wi.changed)
+    gsi_next (iter);
+  return wi.changed;
+}
+
+
 /* Walk each instruction of all basic block and instrument those that
    represent memory references: loads, stores, or function calls.
    In a given basic block, this function avoids instrumenting memory
@@ -2105,7 +2178,10 @@ transform_statements (void)
 	  else if (is_gimple_call (s) && maybe_instrument_call (&i))
 	    /*  Nothing to do as maybe_instrument_call
 		advanced the iterator I.  */;
-	  else
+	  else if (is_gimple_asm (s) && maybe_instrument_asm (&i))
+            /*  Nothing to do as maybe_instrument_asm
+                advanced the iterator I.  */;
+            else
 	    {
 	      /* No instrumentation happened.
 
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 6baaf2a..d709e40 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10182,6 +10182,10 @@ is enabled by default when using @option{-fsanitize=address} option.
 To disable use-after-return detection use 
 @option{--param asan-use-after-return=0}.
 
+@item asan-instrument-asm
+Enable inline assembler support for asan.  This is enabled when using
+@option{-fsanitize=address} and @option{--param asan-instrument-asm=1} option.
+
 @end table
 @end table
 
diff --git a/gcc/gimple.h b/gcc/gimple.h
index 9df45de..c2516f8 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -2395,6 +2395,14 @@ is_gimple_call (const_gimple gs)
   return gimple_code (gs) == GIMPLE_CALL;
 }
 
+/* Return true if GS is a GIMPLE_ASM.  */
+
+static inline bool
+is_gimple_asm (const_gimple gs)
+{
+  return gimple_code (gs) == GIMPLE_ASM;
+}
+
 /* Return the LHS of call statement GS.  */
 
 static inline tree
diff --git a/gcc/params.def b/gcc/params.def
index dd2e2cd..06f29b4 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1064,6 +1064,11 @@ DEFPARAM (PARAM_ASAN_GLOBALS,
          "Enable asan globals protection",
          1, 0, 1)
 
+DEFPARAM (PARAM_ASAN_INSTRUMENT_ASM,
+         "asan-instrument-asm",
+         "Enable asan inline assembly protection",
+         0, 0, 1)
+
 DEFPARAM (PARAM_ASAN_INSTRUMENT_WRITES,
          "asan-instrument-writes",
          "Enable asan store operations protection",
diff --git a/gcc/params.h b/gcc/params.h
index 0d6daa2..e0f468d 100644
--- a/gcc/params.h
+++ b/gcc/params.h
@@ -228,6 +228,8 @@ extern void init_param_values (int *params);
   PARAM_VALUE (PARAM_ASAN_INSTRUMENT_READS)
 #define ASAN_INSTRUMENT_WRITES \
   PARAM_VALUE (PARAM_ASAN_INSTRUMENT_WRITES)
+#define ASAN_INSTRUMENT_ASM \
+  PARAM_VALUE (PARAM_ASAN_INSTRUMENT_ASM)
 #define ASAN_MEMINTRIN \
   PARAM_VALUE (PARAM_ASAN_MEMINTRIN)
 #define ASAN_USE_AFTER_RETURN \
diff --git a/gcc/testsuite/c-c++-common/asan/asm-scalar-1.c b/gcc/testsuite/c-c++-common/asan/asm-scalar-1.c
new file mode 100644
index 0000000..babee83
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/asm-scalar-1.c
@@ -0,0 +1,18 @@
+/* { dg-do run } */
+/* { dg-options "--param asan-instrument-asm=1" } */
+/* { dg-shouldfail "asan" } */
+
+__attribute__ ((noinline))
+void foo (int *p)
+{
+  asm volatile (" "
+                 : "=m" (*p));
+}
+int main ()
+{
+  char a = 129;
+  foo ((int *)&a);
+  return 0;
+}
+
+/* { dg-output "WRITE of size \[0-9\]* at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r)" } */
diff --git a/gcc/testsuite/c-c++-common/asan/asm-scalar-2.c b/gcc/testsuite/c-c++-common/asan/asm-scalar-2.c
new file mode 100644
index 0000000..ff33b10
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/asm-scalar-2.c
@@ -0,0 +1,15 @@
+/* { dg-do run } */
+/* { dg-options "--param asan-instrument-asm=1" } */
+
+__attribute__ ((noinline))
+void foo (int *p)
+{
+  asm volatile (" "
+      	         : "=m" (*p));
+}
+int main ()
+{
+  int a = 129;
+  foo (&a);
+  return 0;
+}
diff --git a/gcc/testsuite/c-c++-common/asan/asm-scalar-3.c b/gcc/testsuite/c-c++-common/asan/asm-scalar-3.c
new file mode 100644
index 0000000..f64f89d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/asm-scalar-3.c
@@ -0,0 +1,16 @@
+/* { dg-do run } */
+/* { dg-options "--param asan-instrument-asm=1" } */
+
+__attribute__ ((noinline))
+void foo (char *p)
+{
+  asm volatile (" "
+      	         : "=r" (*(int *)p));
+}
+int main ()
+{
+  char a = 29;
+  foo (&a);
+  return 0;
+}
+
diff --git a/gcc/testsuite/c-c++-common/asan/asm-struct-1.c b/gcc/testsuite/c-c++-common/asan/asm-struct-1.c
new file mode 100644
index 0000000..2db5e2b
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/asm-struct-1.c
@@ -0,0 +1,21 @@
+/* { dg-do run } */
+/* { dg-options "--param asan-instrument-asm=1" } */
+/* { dg-shouldfail "asan" } */
+
+struct st1 {
+   int a[110];
+};
+
+struct st2 {
+   int a[111];
+};
+
+int main ()
+{
+  struct st1 s1;
+  asm volatile ( " "
+                  : "=m" (*((struct st2 *)&s1)));
+  return 0;
+}
+
+/* { dg-output "ERROR: AddressSanitizer: stack-buffer-overflow on address\[^\n\r]*" } */
diff --git a/gcc/testsuite/c-c++-common/asan/asm-struct-2.c b/gcc/testsuite/c-c++-common/asan/asm-struct-2.c
new file mode 100644
index 0000000..8d8c7a3
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/asm-struct-2.c
@@ -0,0 +1,18 @@
+/* { dg-do run } */
+/* { dg-options "--param asan-instrument-asm=1" } */
+
+struct st1 {
+   int a[111];
+};
+
+struct st2 {
+   int a[111];
+};
+
+int main ()
+{
+  struct st1 s1;
+  asm volatile ( " "
+                  : "=m" (*((struct st2 *)&s1)));
+  return 0;
+}

Re: [PATCH] Inline asm asan instrumentation

[Konstantin Serebryany]

On Wed, May 28, 2014 at 5:33 PM, Marat Zakirov <m.zakirov@samsung.com> wrote:
> Hi all,
>
> Here's a patch for optional Asan instrumentation of inline assembly.
>
> This version scans gimple for GIMPLE_ASMs and performs usual instrumentation
> of arguments with memory constraints ("m", "o", etc.) with fixed size.
>
> Instrumentation is turned off by default.
>
> This was successfully bootstrapped and regtested on x64. I have also
> instrumented and ran ffmpeg regression testsuite (it seems to have quite
> some inline asm).
>
> --Marat

Re: [PATCH] Inline asm asan instrumentation

[Evgeniy Stepanov]

Cool, we don't have this in LLVM-ASan, but we have plans to instrument
inline asm soon (not just constraints).

asm-struct-1.c test looks like a false positive though - the code does
not access any invalid memory, it only does a harmless pointer cast.


On Wed, May 28, 2014 at 10:36 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Wed, May 28, 2014 at 5:33 PM, Marat Zakirov <m.zakirov@samsung.com> wrote:
>> Hi all,
>>
>> Here's a patch for optional Asan instrumentation of inline assembly.
>>
>> This version scans gimple for GIMPLE_ASMs and performs usual instrumentation
>> of arguments with memory constraints ("m", "o", etc.) with fixed size.
>>
>> Instrumentation is turned off by default.
>>
>> This was successfully bootstrapped and regtested on x64. I have also
>> instrumented and ran ffmpeg regression testsuite (it seems to have quite
>> some inline asm).
>>
>> --Marat

Re: [PATCH] Inline asm asan instrumentation

[Jakub Jelinek]

On Wed, May 28, 2014 at 05:33:44PM +0400, Marat Zakirov wrote:
> Here's a patch for optional Asan instrumentation of inline assembly.
> 
> This version scans gimple for GIMPLE_ASMs and performs usual instrumentation
> of arguments with memory constraints ("m", "o", etc.) with fixed size.

That doesn't look right to me.  The fact that some region appears in "m"
doesn't mean the inline asm actually accesses it, it could not touch it at
all, or only some part of it.
If you look e.g. at Linux kernel headers, you'll see lots of
struct __large_struct { unsigned long buf[100]; };
#define __m(x) (*(struct __large_struct __user *)(x))
...
  "m" (__m(addr))
and similar cases, if Asan wants to check that the whole 100*sizeof(long)
region is accessible, it could often just have false positives, because the
inline asm really accesses just some small part of it.

	Jakub

RE: [PATCH] Inline asm asan instrumentation

[Marat Zakirov]

>> asm-struct-1.c test looks like a false positive though - the code does not access any invalid memory, it only does a harmless pointer cast.

It is not. Because st1 have smaller size than st2:

struct st1 {
   int a[110];
}
struct st2 {
   int a[111];
};

And asm constrain was declared as: 

: "=m" (*((struct st2 *)&s1)));

Test violate memory access constrain by cast (struct st2 *)&s1. 
We check only constraints and by such a cast as we think user declare that he want to access full st2 structure which have bigger size than st1. 

-----Original Message-----
From: Evgeniy Stepanov [mailto:eugeni.stepanov@gmail.com] 
Sent: Thursday, May 29, 2014 1:58 PM
To: Konstantin Serebryany
Cc: Marat Zakirov; GCC Patches; Konstantin Serebryany; Jakub Jelinek; Viacheslav Garbuzov; Yuri Gribov; Marat Zakirov
Subject: Re: [PATCH] Inline asm asan instrumentation

Cool, we don't have this in LLVM-ASan, but we have plans to instrument inline asm soon (not just constraints).

asm-struct-1.c test looks like a false positive though - the code does not access any invalid memory, it only does a harmless pointer cast.


On Wed, May 28, 2014 at 10:36 PM, Konstantin Serebryany <konstantin.s.serebryany@gmail.com> wrote:
> On Wed, May 28, 2014 at 5:33 PM, Marat Zakirov <m.zakirov@samsung.com> wrote:
>> Hi all,
>>
>> Here's a patch for optional Asan instrumentation of inline assembly.
>>
>> This version scans gimple for GIMPLE_ASMs and performs usual 
>> instrumentation of arguments with memory constraints ("m", "o", etc.) with fixed size.
>>
>> Instrumentation is turned off by default.
>>
>> This was successfully bootstrapped and regtested on x64. I have also 
>> instrumented and ran ffmpeg regression testsuite (it seems to have 
>> quite some inline asm).
>>
>> --Marat

RE: [PATCH] Inline asm asan instrumentation

[Marat Zakirov]

Actually I do not think that this is good idea to use constraints in a such
arbitrary way. By setting constraints user takes responsibility on himself.
So even if full inline asm support will be done one day, I do think that
checking memory constraints will be still exist. 
It is the same situation as with compiler warnings - sometimes they are
bothering but if you think you do not need them - just do not use them.

-----Original Message-----
From: Jakub Jelinek [mailto:jakub@redhat.com] 
Sent: Thursday, May 29, 2014 2:09 PM
To: Marat Zakirov
Cc: gcc-patches@gcc.gnu.org; 'Konstantin Serebryany'; 'Viacheslav Garbuzov';
'Yuri Gribov'; 'Marat Zakirov'
Subject: Re: [PATCH] Inline asm asan instrumentation

On Wed, May 28, 2014 at 05:33:44PM +0400, Marat Zakirov wrote:
> Here's a patch for optional Asan instrumentation of inline assembly.
> 
> This version scans gimple for GIMPLE_ASMs and performs usual 
> instrumentation of arguments with memory constraints ("m", "o", etc.) with
fixed size.

That doesn't look right to me.  The fact that some region appears in "m"
doesn't mean the inline asm actually accesses it, it could not touch it at
all, or only some part of it.
If you look e.g. at Linux kernel headers, you'll see lots of struct
__large_struct { unsigned long buf[100]; }; #define __m(x) (*(struct
__large_struct __user *)(x)) ...
  "m" (__m(addr))
and similar cases, if Asan wants to check that the whole 100*sizeof(long)
region is accessible, it could often just have false positives, because the
inline asm really accesses just some small part of it.

	Jakub

Re: [PATCH] Inline asm asan instrumentation

[Yury Gribov]

> The fact that some region appears in "m"
> doesn't mean the inline asm actually accesses it, it could not touch it at
> all, or only some part of it.

Do we have precise semantics of "m" written somewhere?
My understanding was that even though asm may not touch buffer at all 
(like e.g. in our tests),
user still declares whole region accessible to compiler.

> if Asan wants to check that the whole 100*sizeof(long)
> region is accessible, it could often just have false positives, because the
> inline asm really accesses just some small part of it.

We've seen this abused (e.g. casting to struct { char x[0xffffffff]; } *)
and that's main reason why we turned this off by default.
On the other we've seen no problems with ffmpeg's testsuite
and ability to detect overflows in inline asm would be rather useful.

Do you see how we could make this more robustly?
We could e.g. only check the first byte
although this wouldn't be as useful.

-Y