[RFC] Introduce -fsanitize=use-after-scope (v2)

On 08/18/2016 03:36 PM, Jakub Jelinek wrote:
> On Thu, May 12, 2016 at 04:12:21PM +0200, Martin Liška wrote:
>> --- a/gcc/asan.c
>> +++ b/gcc/asan.c
>> @@ -243,6 +243,11 @@ static unsigned HOST_WIDE_INT asan_shadow_offset_value;
>>  static bool asan_shadow_offset_computed;
>>  static vec<char *> sanitized_sections;
>>  
>> +/* Set of variable declarations that are going to be guarded by
>> +   use-after-scope sanitizer.  */
>> +
>> +static hash_set<tree> asan_handled_variables (13);
> 
> Doesn't this introduce yet another global ctor?  If yes (and we
> unfortunately have already way too many), I'd strongly prefer to avoid that,
> use pointer to hash_set<tree> or something similar.

Hello

It does, I did pointer in second version of that patch.

> 
>> +/* Depending on POISON flag, emit a call to poison (or unpoison) stack memory
>> +   allocated for local variables, localted in OFFSETS.  LENGTH is number
>> +   of OFFSETS, BASE is the register holding the stack base,
>> +   against which OFFSETS array offsets are relative to.  BASE_OFFSET represents
>> +   an offset requested by alignment and similar stuff.  */
>> +
>> +static void
>> +asan_poison_stack_variables (rtx shadow_base, rtx base,
>> +			     HOST_WIDE_INT base_offset,
>> +			     HOST_WIDE_INT *offsets, int length,
>> +			     tree *decls, bool poison)
>> +{
>> +  if (asan_sanitize_use_after_scope ())
>> +    for (int l = length - 2; l > 0; l -= 2)
>> +      {
> 
> I think this is unfortunate, it leads to:
>         movl    $-235802127, 2147450880(%rax)
>         movl    $-185335552, 2147450884(%rax)
>         movl    $-202116109, 2147450888(%rax)
>         movb    $-8, 2147450884(%rax)
>         movb    $-8, 2147450885(%rax)
> (e.g. on use-after-scope-1.c).
> The asan_emit_stack_protection function already walks all the
> entries in the offsets array in both of the
>   for (l = length; l; l -= 2)
> loops, so please handle the initial poisoning and final unpoisoning there
> as well.  The goal is that for variables that you want poison-after-scope
> at the start of the function (btw, I've noticed that current SVN LLVM
> doesn't bother with it and thus doesn't track "use before scope" (before the
> scope is entered for the first time, maybe we shouldn't either, that would
> catch only compiler bugs rather than user code bugs, right?)) have 0xf8
> on all corresponding bytes including the one that would otherwise have 0x01
> through 0x07.  When unpoisoning at the end of the function, again you should
> combine that with unpoisoning of the red zone and partial zone bytes plus
> the last 0x01 through 0x07, etc.

I also decided to not to handle "use before scope" issues and thus I do not poison
stack variables at the very beginning of a function.

As you noticed, the format stack poisoning/unpoisoning code was kind of ugly.
Current unpoisoning code (trunk version) basically clears the whole shadow memory
for a stack frame except local variables that are not touched by use-after-scope machinery.
That eventually leads to a bit easier code, producing the shadow clearing stuff.

> 
> Plus, as I've mentioned before, it would be nice to optimize - for ASAN_MARK
> unpoison appearing strictly before (i.e. dominating) the first (non-shadow) memory read
> or write in the function (explicit or possible through function calls etc.)
> you really don't need to unpoison (depending on whether we follow LLVM as
> mentioned above then it can be removed without anything, or the decl needs
> to be somehow marked and tell asan_emit_stack_protection it shouldn't poison
> it at the start), and for ASAN_MARK poisoning appearing after the last
> load/store in the function (post dominating those, you don't care about
> noreturn though) you can combine that (remove the ASAN_MARK) with letting
> asan_emit_stack_protection know it doesn't need to unpoison.

Fully agree with that approach, however I would be happy to do that as a follow-up as
it's not going to so trivial..

> 
>> +	    char c = poison ? ASAN_STACK_MAGIC_USE_AFTER_SCOPE : 0;
>> +	    for (unsigned i = 0; i < shadow_size; ++i)
>> +	      {
>> +		emit_move_insn (var_mem, gen_int_mode (c, QImode));
>> +		var_mem = adjust_address (var_mem, QImode, 1);
> 
> When you combine it with the loop, you can also use the infrastructure to
> handle it 4 bytes at a time.

Current implementation can handle up to 4 bytes at a time. I'm wondering we can
do even better for targets with 64-bits memory stores? How can one get such
info about a target?

> 
> Another thing I've noticed is that the inline expansion of
> __asan_unpoison_stack_memory you emit looks buggy.
> In use-after-scope-1.c I see:
>   _9 = (unsigned long) &my_char;
>   _10 = _9 >> 3;
>   _11 = _10 + 2147450880;
>   _12 = (signed char *) _11;
>   MEM[(short int *)_12] = 0;
> 
> That would be fine only for 16 byte long my_char, but we have instead 9 byte
> one.  So I believe in that case we need to store
> 0x00, 0x01 bytes, for little endian thus 0x0100.  You could use for it
> a function similarly to asan_shadow_cst, just build INTEGER_CST rather than
> CONST_INT out of it.  In general, poisioning is storing 0xf8 to all affected
> shadow bytes, unpoisioning should restore the state what we would emit
> without use-after-scope sanitization, which is all but the last byte 0, and
> the last byte 0 only if the var size is a multiple of 8, otherwise number
> of valid bytes (1-7).

Fixed in the newer patch.

> 
> As for the option, it seems clang uses now
> -fsanitize-address-use-after-scope option, while I don't like that much, if
> they have already released some version with that option or if they are
> unwilling to change, I'd go with their option.

I also do not like the option, but 3.9.0 has already the functionality. Thus,
I'm copying LLVM behavior.

> 
>> +         if (flag_stack_reuse != SR_NONE
>> +             && flag_openacc
>> +             && oacc_declare_returns != NULL)
> 
> This actually looks like preexisting OpenACC bug, I doubt the OpenACC
> behavior should depend on -fstack-reuse= setting.

The generated diff for this hunk is bit misleading, I simplified that
in the second version.

> 
> +      bool unpoison_var = asan_poisoned_variables.contains (t);
> +      if (asan_sanitize_use_after_scope ()
> +         && unpoison_var)
> +       asan_poisoned_variables.remove (t);
> 
> Similarly to asan_handled_variables, I'd prefer it to be a pointer to
> hash_set or something similar, so that it costs as few as possible for the
> general case (no sanitization).  Similarly, querying the hash_set even for
> no use-after-scope sanitization looks wrong.

Sure, fixed.

> 
> +         if ((asan_sanitize_stack_p () || asan_sanitize_use_after_scope ())
> 
> I would say if asan_sanitize_stack_p () is false, then we should not be
> doing use-after-scope sanitization (error if user requested that
> explicitly).

Done by adding '&& ASAN_STACK' to asan_sanitize_use_after_scope.

> 
> Don't remember if I've mentioned it earlier, but for vars that are
> TREE_ADDRESSABLE only because of ASAN_MARK calls, we should probably turn
> them non-addressable and remove those ASAN_MARK calls, those shouldn't leak.
> You can have a look at the r237814 change for how similarly
> compare and exchange is special cased for the
> addressables discovery (though, the ASAN_MARK case would be easier, just
> drop it rather than turn it into something different).

I like the approach to not to handle local variables that have address not taken.
My implementation in gimplify.c handles only vars with TREE_ADDRESSABLE set to true,
which should cover all variables which have an address taken in the original code.
I'm aware of cases where one can hit another variable by *(&local_var1 + magic_offset) = 12345,
but that would make the check even more slower.

> 
> I think I miss some testsuite coverage for what has been discussed, stuff
> like:
>   switch (x)
>     {	
>       int a;
>     case 1:
>       int b;
>       foo (&a);
>       foo (&b);
>       /* FALLTHRU */
>     case 2:
>       int c;
>       foo (&a);
>       foo (&b);
>       foo (&c);
> ...
>     }

One can't write such code because of:

use-after-scope-switch.c: In function ‘main’:
use-after-scope-switch.c:16:7: error: a label can only be part of a statement and a declaration is not a statement
       int b;
       ^~~
use-after-scope-switch.c:21:7: error: a label can only be part of a statement and a declaration is not a statement
       int c;
       ^~~

> 
> On use-after-scope-goto-1.c it seems LLVM does a better job, there the goto
> doesn't cross the declaration of any of the a/b/c/d/e/f vars, so doesn't
> need unpoisioning of them; and another testcase is:
> int main(int argc, char **argv)
> {
>   if (argc == 0)
>   {
>     int a;
>     int b;
>     int c;
>     int d;
>     int e;
>     int f;
>     int *ptr;
>     int *ptr2;
>     int *ptr3;
>     int *ptr4;
>     int *ptr5;
>     int *ptr6;
>     label:
>       {
>         a = 123; b = 123; c = 123; d = 123; e = 123; f = 123;
> 	ptr = &a;
>         *ptr = 1;
> 	ptr2 = &b;
>         *ptr2 = 1;
> 	ptr3 = &c;
>         *ptr3 = 1;
> 	ptr4 = &d;
>         *ptr4 = 1;
> 	ptr5 = &e;
>         *ptr5 = 1;
> 	ptr6 = &f;
>         *ptr6 = 1;
> 	return 0;
>       }
>   }
>   else
>     goto label;
> 
>   return 0;
> }
> 
> I believe the C/C++ FEs have warnings/errors for crossing initialization of
> a variable with goto, can't that infrastructure be used also for discovery
> of what variables needs unpoisoning when entering a scope through a goto?
> Of course for something like setjmp (if we need to do anything about it at
> all, maybe longjmp just clears it), or non-local or computed goto we
> probably have to be conservative, but for the common case of local goto
> can't we figure out what needs to be unpoisoned either in the FEs, or during
> gimplification, and emit the unpoisoning not at the label, but on the edge
> between the goto and the label?  

I know that this is still a challenging area. As I've read C/C++ FE, we really have
data structures that are used to identify cross initialization in gotos like:

-Wjump-misses-init
cross.c: In function ‘main’:
cross.c:46:5: warning: jump skips variable initialization [-Wjump-misses-init]
     goto label;
     ^~~~
cross.c:25:7: note: label ‘label’ defined here
       label:
       ^~~~~
cross.c:13:16: note: ‘a’ declared here
       struct A a = { 123 };

However, it's connected to scopes and I'm not sure whether this data structures
are up to date during gimplification and whether on can find a mapping between
LABEL_DECLs and these scopes.

As an initial implementation, I decided to register all LABEL_DECLs that are either
used in goto, or have address taken. For those, I defensively generate unpoisoning
code. Hope it's reasonable initial implementation?

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Apart from that,
following build also succeeded:
./configure --with-build-config="bootstrap-asan" --disable-multilib --disable-werror

Here are some numbers showing how slower is sanitization compared to former -fsanitize=address:
postgres (make check): 26.2 vs. 28.5s
cray (800x600): 9.1s vs. 9.5s
tramp3d: 5x times slower :/ (well, the problem here is that it uses many functions passing args by reference
(all these have address taken); and it does in general very many function calls (stack {un}poisoning cost)).

Martin

> 
> 	Jakub
>

[RFC] Introduce -fsanitize=use-after-scope (v2)

Commit Message

Comments

Patch