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 <mli...@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 <mli...@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
>
>From 6088a945ccbe9bf9c49c8238fe923f63688a778f Mon Sep 17 00:00:00 2001
From: marxin <mli...@suse.cz>
Date: Tue, 25 Sep 2018 10:54:37 +0200
Subject: [PATCH] Make red zone size more flexible for stack variables (PR
sanitizer/81715).
---
gcc/asan.c | 145 ++++++++++++------
gcc/asan.h | 25 +++
gcc/cfgexpand.c | 16 +-
.../c-c++-common/asan/asan-stack-small.c | 28 ++++
4 files changed, 162 insertions(+), 52 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 45906bf8fee..af5b7264e6f 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1155,20 +1155,6 @@ asan_pp_string (pretty_printer *pp)
return build1 (ADDR_EXPR, shadow_ptr_types[0], ret);
}
-/* Return a CONST_INT representing 4 subsequent shadow memory bytes. */
-
-static rtx
-asan_shadow_cst (unsigned char shadow_bytes[4])
-{
- 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);
-}
-
/* Clear shadow memory at SHADOW_MEM, LEN bytes. Can't call a library call here
though. */
@@ -1235,6 +1221,79 @@ shadow_mem_size (unsigned HOST_WIDE_INT size)
return ROUND_UP (size, ASAN_SHADOW_GRANULARITY) / ASAN_SHADOW_GRANULARITY;
}
+#define RZ_BUFFER_SIZE 4
+
+struct asan_redzone_buffer
+{
+ asan_redzone_buffer (rtx shadow_mem, HOST_WIDE_INT prev_offset):
+ m_shadow_mem (shadow_mem), m_prev_offset (prev_offset),
+ m_shadow_bytes (RZ_BUFFER_SIZE)
+ {}
+
+ void emit_redzone_byte (HOST_WIDE_INT offset, unsigned char value);
+ void flush_redzone_payload (void);
+
+ rtx m_shadow_mem;
+ HOST_WIDE_INT m_prev_offset;
+ auto_vec<unsigned char> m_shadow_bytes;
+};
+
+void
+asan_redzone_buffer::emit_redzone_byte (HOST_WIDE_INT offset,
+ unsigned char value)
+{
+ gcc_assert ((offset & (ASAN_SHADOW_GRANULARITY - 1)) == 0);
+ gcc_assert (offset >= m_prev_offset);
+
+ HOST_WIDE_INT off
+ = m_prev_offset + ASAN_SHADOW_GRANULARITY * m_shadow_bytes.length ();
+ if (off == offset)
+ {
+ /* Consecutive shadow memory byte. */
+ m_shadow_bytes.safe_push (value);
+ if (m_shadow_bytes.length () == RZ_BUFFER_SIZE)
+ flush_redzone_payload ();
+ }
+ else
+ {
+ if (!m_shadow_bytes.is_empty ())
+ flush_redzone_payload ();
+
+ /* Adjust m_prev_offset and m_shadow_mem. */
+ m_shadow_mem = adjust_address (m_shadow_mem, VOIDmode,
+ (offset - m_prev_offset)
+ >> ASAN_SHADOW_SHIFT);
+ m_prev_offset = offset;
+ m_shadow_bytes.safe_push (value);
+ }
+}
+
+void
+asan_redzone_buffer::flush_redzone_payload (void)
+{
+ gcc_assert (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN);
+
+ if (m_shadow_bytes.is_empty ())
+ return;
+
+ /* Fill it to RZ_BUFFER_SIZE bytes with zeros if needed. */
+ unsigned l = m_shadow_bytes.length ();
+ for (unsigned i = 0; i <= RZ_BUFFER_SIZE - l; i++)
+ m_shadow_bytes.safe_push (0);
+
+ unsigned HOST_WIDE_INT val = 0;
+ for (unsigned i = 0; i < RZ_BUFFER_SIZE; i++)
+ {
+ unsigned char v = m_shadow_bytes[BYTES_BIG_ENDIAN ? RZ_BUFFER_SIZE - i : i];
+ val |= (unsigned HOST_WIDE_INT)v << (BITS_PER_UNIT * i);
+ }
+
+ rtx c = gen_int_mode (val, SImode);
+ m_shadow_mem = adjust_address (m_shadow_mem, SImode, 0);
+ emit_move_insn (m_shadow_mem, c);
+ m_shadow_bytes.truncate (0);
+}
+
/* Insert code to protect stack vars. The prologue sequence should be emitted
directly, epilogue sequence returned. BASE is the register holding the
stack base, against which OFFSETS array offsets are relative to, OFFSETS
@@ -1256,7 +1315,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;
@@ -1416,51 +1474,46 @@ 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)));
prev_offset = base_offset;
+
+ asan_redzone_buffer rz_buffer (shadow_mem, prev_offset);
for (l = length; l; l -= 2)
{
if (l == 2)
cur_shadow_byte = ASAN_STACK_MAGIC_RIGHT;
offset = offsets[l - 1];
- if ((offset - base_offset) & (ASAN_RED_ZONE_SIZE - 1))
+
+ bool extra_byte = (offset - base_offset) & (ASAN_SHADOW_GRANULARITY - 1);
+ /* 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 (extra_byte)
{
- 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));
- offset = aoff;
+ & ~(ASAN_SHADOW_GRANULARITY - HOST_WIDE_INT_1));
+ rz_buffer.emit_redzone_byte (aoff, offset - aoff);
+ offset = aoff + ASAN_SHADOW_GRANULARITY;
}
- while (offset <= offsets[l - 2] - ASAN_RED_ZONE_SIZE)
+
+ /* Calculate size of red zone payload. */
+ while (offset < offsets[l - 2])
{
- 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;
+ rz_buffer.emit_redzone_byte (offset, cur_shadow_byte);
+ offset += ASAN_SHADOW_GRANULARITY;
}
+
cur_shadow_byte = ASAN_STACK_MAGIC_MIDDLE;
}
+
+ /* Flush red zone buffer. */
+ rz_buffer.flush_redzone_payload ();
+
do_pending_stack_adjust ();
/* Construct epilogue sequence. */
@@ -1519,7 +1572,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_MIN_RED_ZONE_SIZE - HOST_WIDE_INT_1));
if (last_offset + last_size != offset)
{
shadow_mem = adjust_address (shadow_mem, VOIDmode,
@@ -1531,7 +1584,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_MIN_RED_ZONE_SIZE - HOST_WIDE_INT_1))
- offset;
/* Unpoison shadow memory that corresponds to a variable that is
@@ -1552,7 +1605,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_MIN_RED_ZONE_SIZE - HOST_WIDE_INT_1);
}
}
}
diff --git a/gcc/asan.h b/gcc/asan.h
index 2f431b4f938..e1b9b491e67 100644
--- a/gcc/asan.h
+++ b/gcc/asan.h
@@ -53,6 +53,11 @@ extern hash_set <tree> *asan_used_labels;
up to 2 * ASAN_RED_ZONE_SIZE - 1 bytes. */
#define ASAN_RED_ZONE_SIZE 32
+/* Stack variable use more compact red zones. The size includes also
+ size of variable itself. */
+
+#define ASAN_MIN_RED_ZONE_SIZE 16
+
/* Shadow memory values for stack protection. Left is below protected vars,
the first pointer in stack corresponding to that offset contains
ASAN_STACK_FRAME_MAGIC word, the second pointer to a string describing
@@ -102,6 +107,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 occupis on a stack
+ including a space for red zone. */
+
+static inline unsigned HOST_WIDE_INT
+asan_var_and_redzone_size (unsigned HOST_WIDE_INT size)
+{
+ if (size <= 4)
+ return 16;
+ else if (size <= 16)
+ return 32;
+ else if (size <= 128)
+ return size + 32;
+ else if (size <= 512)
+ return size + 64;
+ else if (size <= 4096)
+ return size + 128;
+ else
+ return size + 256;
+}
+
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 21bdcdaeaa3..a839391d298 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -1125,13 +1125,17 @@ 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_MIN_RED_ZONE_SIZE),
!FRAME_GROWS_DOWNWARD);
tree repr_decl = NULL_TREE;
- offset
- = alloc_stack_frame_space (stack_vars[i].size
- + ASAN_RED_ZONE_SIZE,
- MAX (alignb, ASAN_RED_ZONE_SIZE));
+ unsigned HOST_WIDE_INT size
+ = asan_var_and_redzone_size (stack_vars[i].size.to_constant ());
+ if (data->asan_vec.is_empty ())
+ size = MAX (size, ASAN_RED_ZONE_SIZE);
+
+ unsigned HOST_WIDE_INT alignment = MAX (alignb,
+ ASAN_MIN_RED_ZONE_SIZE);
+ offset = alloc_stack_frame_space (size, alignment);
data->asan_vec.safe_push (prev_offset);
/* Allocating a constant amount of space from a constant
@@ -2259,7 +2263,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.1
