On 9/25/18 5:53 PM, Jakub Jelinek wrote: > On Tue, Sep 25, 2018 at 05:26:44PM +0200, Martin Liška wrote: >> 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? > > Yeah, that is a very important optimization. I wasn't using DImode because > at least on x86_64 64-bit constants are quite expensive and on several other > targets even more so, so SImode was a compromise to get size of the prologue > under control and not very slow. What I think we want is figure out ranges
Ah, some time ago, I remember you mentioned the 64-bit constants are expensive (even on x86_64). Btw. it's what clang used for the red zone instrumentation. > of shadow bytes we want to initialize and the values we want to store there, > perhaps take also into account strict alignment vs. non-strict alignment, > and perform kind of store merging for it. Given that 2 shadow bytes would > be only used for the very small variables (<=4 bytes in size, so <= 0.5 > bytes of shadow), we'd just need a way to remember the 2 shadow bytes across > handling adjacent vars and store it together. Agree, it's implemented in next version of patch. > > I think we want to introduce some define for minimum red zone size and use > it instead of the granularity (granularity is 8 bytes, but minimum red zone > size if we count into it also the very small variable size is 16 bytes). > >> --- 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) > > The argument needs to be UHWI, otherwise you do a wrong thing for > say 4GB + 4 bytes long variable. Ditto the result. > >> +{ >> + 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; > > I'd prefer size + const instead of const + size operand order. > >> @@ -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), > > Use that ASAN_MIN_RED_ZONE_SIZE (16) here. > >> !FRAME_GROWS_DOWNWARD); >> tree repr_decl = NULL_TREE; >> + poly_uint64 size = asan_var_and_redzone_size >> (stack_vars[i].size.to_constant ()); > > Too long line. Two spaces instead of one. Why poly_uint64? > Plus, perhaps if data->asan_vec is empty (i.e. when assigning the topmost > automatic variable in a frame), we should ensure that size is at least > 2 * ASAN_RED_ZONE_SIZE (or just 1 * ASAN_RED_ZONE_SIZE). > >> 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)); > > Again, too long line and we want 16 instead of 8 here too. >> >> 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) > > and here too. > > Jakub > The rest is also implemented as requested. I'm testing Linux kernel now, will send stats to the PR created for it. Patch survives testing on x86_64-linux-gnu. Martin
>From acbea3a2127a5eb19e23a202010847e098cf8ce8 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/ChangeLog: 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. --- gcc/asan.c | 108 +++++++++++------- gcc/asan.h | 25 ++++ gcc/cfgexpand.c | 16 ++- .../c-c++-common/asan/asan-stack-small.c | 28 +++++ 4 files changed, 132 insertions(+), 45 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..6552ca66132 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1155,18 +1155,34 @@ 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. */ +/* Emit red zones payload that started at SHADOW_MEM address. + SHADOW_BYTES contains payload that should be stored. */ static rtx -asan_shadow_cst (unsigned char shadow_bytes[4]) +asan_emit_redzone_payload (rtx shadow_mem, + auto_vec<unsigned char> &shadow_bytes) { - 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); + while (!shadow_bytes.is_empty ()) + { + unsigned l = shadow_bytes.length (); + unsigned chunk = l >= 4 ? 4 : (l >= 2 ? 2 : 1); + machine_mode mode = chunk == 4 ? SImode : (chunk == 2 ? HImode : QImode); + shadow_mem = adjust_address (shadow_mem, mode, 0); + + unsigned HOST_WIDE_INT val = 0; + gcc_assert (WORDS_BIG_ENDIAN == BYTES_BIG_ENDIAN); + for (unsigned i = 0; i < chunk; i++) + { + unsigned char v = shadow_bytes[BYTES_BIG_ENDIAN ? chunk - i : i]; + val |= (unsigned HOST_WIDE_INT)v << (BITS_PER_UNIT * i); + } + rtx c = gen_int_mode (val, mode); + emit_move_insn (shadow_mem, c); + shadow_mem = adjust_address (shadow_mem, VOIDmode, chunk); + shadow_bytes.block_remove (0, chunk); + } + + return shadow_mem; } /* Clear shadow memory at SHADOW_MEM, LEN bytes. Can't call a library call here @@ -1256,7 +1272,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,49 +1413,64 @@ 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; + + auto_vec<unsigned char> shadow_bytes (64); 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 merging_p = !shadow_bytes.is_empty (); + 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)); + & ~(ASAN_SHADOW_GRANULARITY - HOST_WIDE_INT_1)); + shadow_bytes.quick_push (offset - aoff); offset = aoff; } - while (offset <= offsets[l - 2] - ASAN_RED_ZONE_SIZE) + + /* Adjust shadow memory to beginning of shadow memory + (or one byte earlier). */ + if (!merging_p) + shadow_mem = adjust_address (shadow_mem, VOIDmode, + (offset - prev_offset) + >> ASAN_SHADOW_SHIFT); + + if (extra_byte) + offset += ASAN_SHADOW_GRANULARITY; + + /* Calculate size of red zone payload. */ + while (offset < offsets[l - 2]) { - shadow_mem = adjust_address (shadow_mem, VOIDmode, - (offset - prev_offset) - >> ASAN_SHADOW_SHIFT); + shadow_bytes.quick_push (cur_shadow_byte); + offset += ASAN_SHADOW_GRANULARITY; + } + + /* Do simple store merging for 2 adjacent small variables + that will need 4 bytes in total to emit red zones. */ + if (shadow_bytes.length () == 2 + && l >= 3 + && ((unsigned HOST_WIDE_INT)(offsets[l - 3] - offsets[l - 2]) + < ASAN_SHADOW_GRANULARITY)) + ; + else + { + shadow_mem = asan_emit_redzone_payload (shadow_mem, shadow_bytes); 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; } + cur_shadow_byte = ASAN_STACK_MAGIC_MIDDLE; } do_pending_stack_adjust (); @@ -1501,7 +1531,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, @@ -1513,7 +1543,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 @@ -1534,7 +1564,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 35ca276e4ad..1a1abe1f6a2 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 @@ -2254,7 +2258,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