[PATCH] Come up with --param asan-stack-small-redzone (PR sanitizer/81715).

[Martin Liška]

On 11/28/18 12:41 PM, Jakub Jelinek wrote:
> On Wed, Sep 26, 2018 at 11:33:25AM +0200, Martin Liška wrote:
>> 2018-09-26  Martin Liska  <mliska@suse.cz>
>>
>> 	PR sanitizer/81715
>> 	* asan.c (asan_shadow_cst): Remove.
>> 	(asan_emit_redzone_payload): New.
>> 	(asan_emit_stack_protection): Make it more
>> 	flexible to support arbitrary size of red zones.
>> 	* asan.h (ASAN_MIN_RED_ZONE_SIZE): New.
>> 	(asan_var_and_redzone_size): Likewise.
>> 	* cfgexpand.c (expand_stack_vars): Reserve size
>> 	for stack vars according to asan_var_and_redzone_size.
>> 	(expand_used_vars): Make smaller offset based
>> 	on ASAN_SHADOW_GRANULARITY.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2018-09-26  Martin Liska  <mliska@suse.cz>
>>
>> 	PR sanitizer/81715
>> 	* c-c++-common/asan/asan-stack-small.c: New test.
> 
> Sorry for the delay.  I had a quick look.  Using:
> struct S { char a[32]; };
> void bar (void *, void *, void *, void *);
> 
> int
> foo (void)
> {
>   struct S a, b, c, d;
>   bar (&a, &b, &c, &d);
>   return 0;
> }
> 
> int
> baz (void)
> {
>   char a;
>   short b;
>   int c;
>   long long d;
>   bar (&a, &b, &c, &d);
>   return 0;
> }
> -O2 -fsanitize=address, I see that foo is identical before/after your patch,
> perfect.
> Looking at baz, I see issues though:
> .LC2:
>         .string "4 32 1 4 a:15 48 2 4 b:16 64 4 4 c:17 80 8 4 d:18"
> ...
>         movq    %rbx, %rbp
>         movl    $-3580, %edx
>         movl    $-3085, %ecx
>         movq    $1102416563, (%rbx)
>         shrq    $3, %rbp
>         movq    $.LC2, 8(%rbx)
>         leaq    48(%rbx), %rsi
>         leaq    32(%rbx), %rdi
>         movq    $.LASANPC1, 16(%rbx)
>         movw    %dx, 2147450888(%rbp)
>         leaq    64(%rbx), %rdx
>         movw    %cx, 2147450891(%rbp)
>         leaq    80(%rbx), %rcx
>         movl    $-235802127, 2147450880(%rbp)
>         movl    $-234687999, 2147450884(%rbp)
>         movb    $-13, 2147450893(%rbp)
>         call    bar
>         cmpq    %rbx, %r12
>         jne     .L15
>         movq    $0, 2147450880(%rbp)
>         xorl    %eax, %eax
>         movl    $0, 2147450888(%rbp)
>         movw    %ax, 2147450892(%rbp)
> So, looks like the shadow at (%rbx>>3)+0x7fff8000 is:
> / 2147450880(%rbp)
> |           / 2147450884(%rbp)
> |           |           / 2147450888(%rbp)
> |           |           |           / 2147450892(%rbp)
> |           |           |           |
> f1 f1 f1 f1 01 f2 02 f2 04 f2 00 f3 f3 f3

Hi.

> 
> Two problems, it uses unconditionally unaligned stores, without
> checking if the target supports them at all (in this case it does).
> And, it doesn't check if it wouldn't be more efficient to use
> 32-bit stores. 

Ok, so now we reduced the alignment of stack variables to ASAN_MIN_RED_ZONE_SIZE (16).
What options do wehave when we emit the red zones? The only guarantee we have is
that in shadow memory we are aligned to at least 2. Should byte-based emission used
for STRICT_ALIGNMENT targets?

>  It isn't that we want the gaps to have whatever
> value the shadow memory contained before, we want it to be 0 and just rely
> on it being zero from earlier.
> Another question is if it wouldn't be worth to ensure the variable area is
> always a multiple of 32 bytes (thus the corresponding shadow always multiple
> of 4 bytes).  Then, for this testcase, you'd store:
> $-235802127, 2147450880(%rbp)	// 0xf1f1f1f1
> $-234687999, 2147450884(%rbp)   // 0xf202f201
> $-218041852, 2147450888(%rbp)   // 0xf300f204
> $-202116109, 2147450892(%rbp)   // 0xf3f3f3f3
> For the single char/short/int/long var in a function case you'd still emit
> just f1 f1 f1 f1 0[1240] f3 f3 f3
> and the number of final f3 bytes would be from 3 to 6 (or 2 to 5), 1 is
> probably too few.

I like the idea, so that can be achieved by setting alignment of the last stack
variable to 32, right?

> 
> Rather than special-casing the two very small adjacent vars case,
> just use a rolling handling of the shadow bytes, if you fill all 4,
> emit immediately, otherwise queue later and flush if the next shadow offset
> is outside of the 4 bytes.

I rewrote it and I'm sending untested patch (however surviving asan.exp tests).
I must agree the code is now much more readable.

>  Either always use SImode stores, or check rtx

I'm now using only SImode stores, which should be the same as before the patch.
The complication now we have is that the guarantee alignment is only 2B in shadow
mem.

Martin

> costs; if the 32-bit constants you'd store is as expensive or less than the
> 16-bit or 8-bit constant, use 32-bit store, otherwise use a 16-bit or 8-bit
> one.  If you want to complicate it further, allow unaligned stores on
> targets that do allow them, but use them with care, if you could use same
> amount of aligned stores with the same cost of constants, prefer aligned
> ones.
> 
> 	Jakub
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-red-zone-size-more-flexible-for-stack-variables.patch
Type: text/x-patch
Size: 11446 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20181129/d03446f8/attachment.bin>