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

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Sep 25, 2018 at 12:10:42PM +0200, Martin Liška wrote: On 9/25/18
> 11:24 AM, Jakub Jelinek wrote:
> > On Tue, Sep 25, 2018 at 11:05:30AM +0200, Martin Liška wrote:
> >> As requested in PR81715, GCC emits bigger middle redzones for small variables.
> >> It's analyzed in following comment: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715#c28
> > 
> > First of all, does LLVM make the variable sized red zone size only for
> > automatic variables, or also for global/local statics, or for alloca?
> 
> Yes, definitely for global variables, as seen here:
> 
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:
>   2122      Type *Ty = G->getValueType();
>   2123      uint64_t SizeInBytes = DL.getTypeAllocSize(Ty);
>   2124      uint64_t MinRZ = MinRedzoneSizeForGlobal();
>   2125      // MinRZ <= RZ <= kMaxGlobalRedzone
>   2126      // and trying to make RZ to be ~ 1/4 of SizeInBytes.
>   2127      uint64_t RZ = std::max(
>   2128          MinRZ, std::min(kMaxGlobalRedzone, (SizeInBytes / MinRZ / 4) * MinRZ));
>   2129      uint64_t RightRedzoneSize = RZ;
>   2130      // Round up to MinRZ
>   2131      if (SizeInBytes % MinRZ) RightRedzoneSize += MinRZ - (SizeInBytes % MinRZ);
>   2132      assert(((RightRedzoneSize + SizeInBytes) % MinRZ) == 0);
>   2133      Type *RightRedZoneTy = ArrayType::get(IRB.getInt8Ty(), RightRedzoneSize);
> 
> So roughly to SizeInBytes / 4. Confirmed:

Changing non-automatic vars will be certainly harder.  Let's do it later.

> > Have you considered also making the red zones larger for very large
> > variables?
> 
> I was mainly focused on shrinking as that's limiting usage of asan-stack in KASAN.
> But I'm open for it. Would you follow what LLVM does, or do you have a specific idea how
> to growth?

Dunno if they've done some analysis before picking the current sizes, unless
we you do some I'd follow their numbers, it doesn't look totally
unreasonable, a compromise between not wasting too much for more frequent
smaller vars and for larger vars catching even larger out of bound accesses.

> > What exactly would need changing to support the 12-15 bytes long red zones
> > for 4-1 bytes long automatic vars?
> > Just asan_emit_stack_protection or something other?
> 
> Primarily this function, that would need a generalization. Apart from that we're
> also doing alignment to ASAN_RED_ZONE_SIZE:
> 
> 	      prev_offset = align_base (prev_offset,
> 					MAX (alignb, ASAN_RED_ZONE_SIZE),
> 					!FRAME_GROWS_DOWNWARD);
> 
> Maybe it has consequences I don't see right now?

Actually, I think even:
+                 && i != n - 1                                                                                                                    
in your patch isn't correct, vars could be last even if they aren't == n - 1
or vice versa, the sorting is done by many factors, vars can go into
multiple buckets based on predicate etc.
So, rather than trying to guess what is last here it should be left to
expand_used_vars for one side, and perhaps based on whether any vars were
placed at all on the other side (don't remember if asan supports anything
but FRAME_GROWS_DOWNWARD).

> First feedback is positive:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81715#c30
> 
> It's questionable whether handling of variables 1-4B wide worth further changes.

I'd think the really small vars are quite common, isn't that the case (sure,
address taken ones will be less common, but still not rare).

	Jakub