On 9/25/18 12:40 PM, Jakub Jelinek wrote:
> 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.
Agree with that!
>
>>> 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).
OK, it only affects size of red redzone, that can be bigger.
>
>> 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).
So I decided to write the patch properly, I have a working version that survives
asan.exp tests. The code that creates red-zones is more generic and all stack
vars are now aligned just to ASAN_SHADOW_GRANULARITY.
The only missing piece is how to implement asan_emit_redzone_payload more smart.
It means doing memory stores with 8,4,2,1 sizes in order to reduce # of insns.
Do we have somewhere a similar code?
Do you like the generalization I did in general?
Thanks,
Maritn
>
> Jakub
>
>From eb1a81fb08b288a8a4e0b2c5a055931e027f233c Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Tue, 25 Sep 2018 10:54:37 +0200
Subject: [PATCH] First semi-working version.
---
gcc/asan.c | 90 ++++++++++---------
gcc/asan.h | 20 +++++
gcc/cfgexpand.c | 10 +--
.../c-c++-common/asan/asan-stack-small.c | 28 ++++++
4 files changed, 102 insertions(+), 46 deletions(-)
create mode 100644 gcc/testsuite/c-c++-common/asan/asan-stack-small.c
diff --git a/gcc/asan.c b/gcc/asan.c
index 235e219479d..5b8ae77b0c6 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1158,15 +1158,24 @@ asan_pp_string (pretty_printer *pp)
/* Return a CONST_INT representing 4 subsequent shadow memory bytes. */
static rtx
-asan_shadow_cst (unsigned char shadow_bytes[4])
+asan_emit_redzone_payload (rtx shadow_mem, unsigned int payload_size,
+ unsigned char shadow_byte,
+ unsigned extra_shadow_byte)
{
- int i;
- unsigned HOST_WIDE_INT val = 0;
- gcc_assert (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN);
- for (i = 0; i < 4; i++)
- val |= (unsigned HOST_WIDE_INT) shadow_bytes[BYTES_BIG_ENDIAN ? 3 - i : i]
- << (BITS_PER_UNIT * i);
- return gen_int_mode (val, SImode);
+ if (extra_shadow_byte)
+ {
+ emit_move_insn (shadow_mem, gen_int_mode (extra_shadow_byte, QImode));
+ shadow_mem = adjust_address (shadow_mem, VOIDmode, 1);
+ --payload_size;
+ }
+
+ for (unsigned i = 0; i < payload_size; i++)
+ {
+ emit_move_insn (shadow_mem, gen_int_mode (shadow_byte, QImode));
+ shadow_mem = adjust_address (shadow_mem, VOIDmode, 1);
+ }
+
+ return shadow_mem;
}
/* Clear shadow memory at SHADOW_MEM, LEN bytes. Can't call a library call here
@@ -1256,7 +1265,6 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
rtx_code_label *lab;
rtx_insn *insns;
char buf[32];
- unsigned char shadow_bytes[4];
HOST_WIDE_INT base_offset = offsets[length - 1];
HOST_WIDE_INT base_align_bias = 0, offset, prev_offset;
HOST_WIDE_INT asan_frame_size = offsets[0] - base_offset;
@@ -1398,7 +1406,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
+ (base_align_bias >> ASAN_SHADOW_SHIFT));
gcc_assert (asan_shadow_set != -1
&& (ASAN_RED_ZONE_SIZE >> ASAN_SHADOW_SHIFT) == 4);
- shadow_mem = gen_rtx_MEM (SImode, shadow_base);
+ shadow_mem = gen_rtx_MEM (QImode, shadow_base);
set_mem_alias_set (shadow_mem, asan_shadow_set);
if (STRICT_ALIGNMENT)
set_mem_align (shadow_mem, (GET_MODE_ALIGNMENT (SImode)));
@@ -1408,39 +1416,39 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
if (l == 2)
cur_shadow_byte = ASAN_STACK_MAGIC_RIGHT;
offset = offsets[l - 1];
- if ((offset - base_offset) & (ASAN_RED_ZONE_SIZE - 1))
+
+ char extra_shadow_byte = 0;
+
+ /* If a red-zone is not aligned to ASAN_SHADOW_GRANULARITY then
+ the previous stack variable has size % ASAN_SHADOW_GRANULARITY != 0.
+ In that case we have to emit one extra byte that will describe
+ how many bytes (our of ASAN_SHADOW_GRANULARITY) can be accessed. */
+ if ((offset - base_offset) & (ASAN_SHADOW_GRANULARITY - 1))
{
- int i;
HOST_WIDE_INT aoff
= base_offset + ((offset - base_offset)
- & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
- shadow_mem = adjust_address (shadow_mem, VOIDmode,
- (aoff - prev_offset)
- >> ASAN_SHADOW_SHIFT);
- prev_offset = aoff;
- for (i = 0; i < 4; i++, aoff += ASAN_SHADOW_GRANULARITY)
- if (aoff < offset)
- {
- if (aoff < offset - (HOST_WIDE_INT)ASAN_SHADOW_GRANULARITY + 1)
- shadow_bytes[i] = 0;
- else
- shadow_bytes[i] = offset - aoff;
- }
- else
- shadow_bytes[i] = ASAN_STACK_MAGIC_MIDDLE;
- emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
+ & ~(ASAN_SHADOW_GRANULARITY - HOST_WIDE_INT_1));
+ extra_shadow_byte = offset - aoff;
offset = aoff;
}
- while (offset <= offsets[l - 2] - ASAN_RED_ZONE_SIZE)
- {
- shadow_mem = adjust_address (shadow_mem, VOIDmode,
- (offset - prev_offset)
- >> ASAN_SHADOW_SHIFT);
- prev_offset = offset;
- memset (shadow_bytes, cur_shadow_byte, 4);
- emit_move_insn (shadow_mem, asan_shadow_cst (shadow_bytes));
- offset += ASAN_RED_ZONE_SIZE;
- }
+
+
+ /* Adjust shadow memory to beginning of shadow memory
+ (or one byte earlier). */
+ shadow_mem = adjust_address (shadow_mem, VOIDmode,
+ (offset - prev_offset)
+ >> ASAN_SHADOW_SHIFT);
+
+ /* Calculate size of red zone payload. */
+ unsigned int payload_size
+ = (offsets[l - 2] - offset) / ASAN_SHADOW_GRANULARITY;
+ offset += payload_size * ASAN_SHADOW_GRANULARITY;
+
+ /* Emit red zone content. */
+ shadow_mem = asan_emit_redzone_payload (shadow_mem, payload_size,
+ cur_shadow_byte, extra_shadow_byte);
+
+ prev_offset = offset;
cur_shadow_byte = ASAN_STACK_MAGIC_MIDDLE;
}
do_pending_stack_adjust ();
@@ -1501,7 +1509,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
for (l = length; l; l -= 2)
{
offset = base_offset + ((offsets[l - 1] - base_offset)
- & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
+ & ~(ASAN_SHADOW_GRANULARITY - HOST_WIDE_INT_1));
if (last_offset + last_size != offset)
{
shadow_mem = adjust_address (shadow_mem, VOIDmode,
@@ -1513,7 +1521,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
last_size = 0;
}
last_size += base_offset + ((offsets[l - 2] - base_offset)
- & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1))
+ & ~(ASAN_SHADOW_GRANULARITY - HOST_WIDE_INT_1))
- offset;
/* Unpoison shadow memory that corresponds to a variable that is
@@ -1534,7 +1542,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb,
"%s (%" PRId64 " B)\n", n, size);
}
- last_size += size & ~(ASAN_RED_ZONE_SIZE - HOST_WIDE_INT_1);
+ last_size += size & ~(ASAN_SHADOW_GRANULARITY - HOST_WIDE_INT_1);
}
}
}
diff --git a/gcc/asan.h b/gcc/asan.h
index 2f431b4f938..c0736a0cb6f 100644
--- a/gcc/asan.h
+++ b/gcc/asan.h
@@ -102,6 +102,26 @@ asan_red_zone_size (unsigned int size)
return c ? 2 * ASAN_RED_ZONE_SIZE - c : ASAN_RED_ZONE_SIZE;
}
+/* Return how much a stack variable occupy on a stack
+ including a space for redzone. */
+
+static inline unsigned int
+asan_var_and_redzone_size (unsigned int size)
+{
+ if (size <= 4)
+ return 16;
+ else if (size <= 16)
+ return 32;
+ else if (size <= 128)
+ return 32 + size;
+ else if (size <= 512)
+ return 64 + size;
+ else if (size <= 4096)
+ return 128 + size;
+ else
+ return 256 + size;
+}
+
extern bool set_asan_shadow_offset (const char *);
extern void set_sanitized_sections (const char *);
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 35ca276e4ad..e84c82599e6 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1125,13 +1125,13 @@ expand_stack_vars (bool (*pred) (size_t), struct stack_vars_data *data)
&& stack_vars[i].size.is_constant ())
{
prev_offset = align_base (prev_offset,
- MAX (alignb, ASAN_RED_ZONE_SIZE),
+ MAX (alignb, ASAN_SHADOW_GRANULARITY),
!FRAME_GROWS_DOWNWARD);
tree repr_decl = NULL_TREE;
+ poly_uint64 size = asan_var_and_redzone_size (stack_vars[i].size.to_constant ());
offset
- = alloc_stack_frame_space (stack_vars[i].size
- + ASAN_RED_ZONE_SIZE,
- MAX (alignb, ASAN_RED_ZONE_SIZE));
+ = alloc_stack_frame_space (size,
+ MAX (alignb, ASAN_SHADOW_GRANULARITY));
data->asan_vec.safe_push (prev_offset);
/* Allocating a constant amount of space from a constant
@@ -2254,7 +2254,7 @@ expand_used_vars (void)
& ~(data.asan_alignb - HOST_WIDE_INT_1)) - sz;
/* Allocating a constant amount of space from a constant
starting offset must give a constant result. */
- offset = (alloc_stack_frame_space (redzonesz, ASAN_RED_ZONE_SIZE)
+ offset = (alloc_stack_frame_space (redzonesz, ASAN_SHADOW_GRANULARITY)
.to_constant ());
data.asan_vec.safe_push (prev_offset);
data.asan_vec.safe_push (offset);
diff --git a/gcc/testsuite/c-c++-common/asan/asan-stack-small.c b/gcc/testsuite/c-c++-common/asan/asan-stack-small.c
new file mode 100644
index 00000000000..11a56b8db4c
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/asan-stack-small.c
@@ -0,0 +1,28 @@
+/* { dg-do run } */
+
+char *pa;
+char *pb;
+char *pc;
+
+void access (volatile char *ptr)
+{
+ *ptr = 'x';
+}
+
+int main (int argc, char **argv)
+{
+ char a;
+ char b;
+ char c;
+
+ pa = &a;
+ pb = &b;
+ pc = &c;
+
+ access (pb);
+ access (pc);
+ // access 'b' here
+ access (pa + 32);
+
+ return 0;
+}
--
2.19.0
