Re: [PATCH] Add missing store in emission of asan_stack_free.
On Wed, Jun 10, 2020 at 01:14:59PM +0200, Martin Liška wrote: > >From 4d2e0b1e87b08ec21fd82144f00d364687030706 Mon Sep 17 00:00:00 2001 > From: Martin Liska > Date: Tue, 19 May 2020 16:57:56 +0200 > Subject: [PATCH] Add missing store in emission of asan_stack_free. > > gcc/ChangeLog: > > 2020-05-19 Martin Liska > > PR sanitizer/94910 > * asan.c (asan_emit_stack_protection): Emit > also **SavedFlagPtr(FakeStack) = 0 in order to release > a stack frame. Please adjust the ChangeLog too. Ok with that change. Jakub
Re: [PATCH] Add missing store in emission of asan_stack_free.
On 6/10/20 12:08 PM, Jakub Jelinek wrote: On Wed, Jun 10, 2020 at 11:49:01AM +0200, Martin Liška wrote: - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , -BITS_PER_UNIT, true, RETURN_BEGIN); + { + /* Emit: + memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); + **SavedFlagPtr(FakeStack) = 0 SavedFlagPtr has two arguments, doesn't it? Good point, I copied that from llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp which has it wrong. Fixed that. + */ + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , + BITS_PER_UNIT, true, RETURN_BEGIN); + + unsigned HOST_WIDE_INT offset + = (1 << (use_after_return_class + 6)); + offset -= GET_MODE_SIZE (ptr_mode); So, mem here is a MEM into which we stored ASAN_STACK_RETIRED_MAGIC. Ok, we should rather start from 'base'. + mem = adjust_address (mem, ptr_mode, offset); This adds offset to it and changes mode to ptr_mode. So, mem is now *(ptr_mode)(_mem + offset) + rtx addr = gen_reg_rtx (ptr_mode); + emit_move_insn (addr, mem); We load that value. + mem = gen_rtx_MEM (ptr_mode, addr); + mem = adjust_address (mem, QImode, 0); And here I'm lost why you do that. If you want to store a single byte into what it points to, then why don't you just mem = gen_rtx_MEM (QImode, addr); instead of the above two lines? Because I'm not so much familiar with RTL ;) adjust_address will return a MEM like the above, with offset not adjusted (as the addition is 0) and mode changed to QImode, but there is no reason not to create it already in QImode. All right. What about this? Martin + emit_move_insn (mem, const0_rtx); + } else if (use_after_return_class >= 5 || !set_storage_via_setmem (shadow_mem, GEN_INT (sz), -- 2.26.2 Jakub >From 4d2e0b1e87b08ec21fd82144f00d364687030706 Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Tue, 19 May 2020 16:57:56 +0200 Subject: [PATCH] Add missing store in emission of asan_stack_free. gcc/ChangeLog: 2020-05-19 Martin Liska PR sanitizer/94910 * asan.c (asan_emit_stack_protection): Emit also **SavedFlagPtr(FakeStack) = 0 in order to release a stack frame. --- gcc/asan.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/gcc/asan.c b/gcc/asan.c index c9872f1b007..e015fa3ec9b 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1598,8 +1598,24 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, if (use_after_return_class < 5 && can_store_by_pieces (sz, builtin_memset_read_str, , BITS_PER_UNIT, true)) - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , - BITS_PER_UNIT, true, RETURN_BEGIN); + { + /* Emit: + memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); + **SavedFlagPtr(FakeStack, class_id) = 0 + */ + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , + BITS_PER_UNIT, true, RETURN_BEGIN); + + unsigned HOST_WIDE_INT offset + = (1 << (use_after_return_class + 6)); + offset -= GET_MODE_SIZE (ptr_mode); + mem = gen_rtx_MEM (ptr_mode, base); + mem = adjust_address (mem, ptr_mode, offset); + rtx addr = gen_reg_rtx (ptr_mode); + emit_move_insn (addr, mem); + mem = gen_rtx_MEM (QImode, addr); + emit_move_insn (mem, const0_rtx); + } else if (use_after_return_class >= 5 || !set_storage_via_setmem (shadow_mem, GEN_INT (sz), -- 2.26.2
Re: [PATCH] Add missing store in emission of asan_stack_free.
On Wed, Jun 10, 2020 at 11:49:01AM +0200, Martin Liška wrote: > - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , > - BITS_PER_UNIT, true, RETURN_BEGIN); > + { > + /* Emit: > +memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); > +**SavedFlagPtr(FakeStack) = 0 SavedFlagPtr has two arguments, doesn't it? > + */ > + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , > +BITS_PER_UNIT, true, RETURN_BEGIN); > + > + unsigned HOST_WIDE_INT offset > + = (1 << (use_after_return_class + 6)); > + offset -= GET_MODE_SIZE (ptr_mode); So, mem here is a MEM into which we stored ASAN_STACK_RETIRED_MAGIC. > + mem = adjust_address (mem, ptr_mode, offset); This adds offset to it and changes mode to ptr_mode. So, mem is now *(ptr_mode)(_mem + offset) > + rtx addr = gen_reg_rtx (ptr_mode); > + emit_move_insn (addr, mem); We load that value. > + mem = gen_rtx_MEM (ptr_mode, addr); > + mem = adjust_address (mem, QImode, 0); And here I'm lost why you do that. If you want to store a single byte into what it points to, then why don't you just mem = gen_rtx_MEM (QImode, addr); instead of the above two lines? adjust_address will return a MEM like the above, with offset not adjusted (as the addition is 0) and mode changed to QImode, but there is no reason not to create it already in QImode. > + emit_move_insn (mem, const0_rtx); > + } >else if (use_after_return_class >= 5 > || !set_storage_via_setmem (shadow_mem, > GEN_INT (sz), > -- > 2.26.2 > Jakub
Re: [PATCH] Add missing store in emission of asan_stack_free.
On 6/10/20 10:42 AM, Jakub Jelinek wrote: E.g. we just shouldn't reuse MEMs (even after adjusting them) from different indirection levels because we risk some attributes (alias set, MEM_EXPR, whatever else) will stay around from the different indirection level. All right, what about the updated patch? I must confess that RTL instruction emission is not my strength. Martin >From 16e46a532c059930887bc30f82c3054a75a5a56d Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Tue, 19 May 2020 16:57:56 +0200 Subject: [PATCH] Add missing store in emission of asan_stack_free. gcc/ChangeLog: 2020-05-19 Martin Liska PR sanitizer/94910 * asan.c (asan_emit_stack_protection): Emit also **SavedFlagPtr(FakeStack) = 0 in order to release a stack frame. --- gcc/asan.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/gcc/asan.c b/gcc/asan.c index c9872f1b007..232341f5c4b 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1598,8 +1598,24 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, if (use_after_return_class < 5 && can_store_by_pieces (sz, builtin_memset_read_str, , BITS_PER_UNIT, true)) - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , - BITS_PER_UNIT, true, RETURN_BEGIN); + { + /* Emit: + memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); + **SavedFlagPtr(FakeStack) = 0 + */ + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , + BITS_PER_UNIT, true, RETURN_BEGIN); + + unsigned HOST_WIDE_INT offset + = (1 << (use_after_return_class + 6)); + offset -= GET_MODE_SIZE (ptr_mode); + mem = adjust_address (mem, ptr_mode, offset); + rtx addr = gen_reg_rtx (ptr_mode); + emit_move_insn (addr, mem); + mem = gen_rtx_MEM (ptr_mode, addr); + mem = adjust_address (mem, QImode, 0); + emit_move_insn (mem, const0_rtx); + } else if (use_after_return_class >= 5 || !set_storage_via_setmem (shadow_mem, GEN_INT (sz), -- 2.26.2
Re: [PATCH] Add missing store in emission of asan_stack_free.
On Wed, Jun 10, 2020 at 10:24:59AM +0200, Martin Liška wrote: > > > This doesn't look correct to me. > > > I'd think the first adjust_address should be > > > mem = adjust_address (mem, ptr_mode, offset); > > > which will give you a MEM with ptr_mode which has SavedFlagPtr(FakeStack) > > > address, i.e. *SavedFlagPtr(FakeStack). > > > Next, you want to load that into some temporary, so e.g. > > > rtx addr = gen_reg_rtx (ptr_mode); > > > emit_move_insn (addr, mem); > > > next you need to convert that ptr_mode to Pmode if needed, so something > > > like > > > addr = convert_memory_address (Pmode, addr); > > > and finally: > > > mem = gen_rtx_MEM (QImode, addr); > > > emit_move_insn (mem, const0_rtx); > > > Completely untested. > > > > This is not correct. With your suggestion I have: > > > > int foo(int index) > > { > > int a[100]; > > return a[index]; > > } > > > > $ diff -u before.s after.s > > --- before.s 2020-06-01 15:15:22.634337654 +0200 > > +++ after.s 2020-06-01 15:16:32.205711511 +0200 > > @@ -81,8 +81,7 @@ > > movq %rdi, 2147450920(%rax) > > movq %rsi, 2147450928(%rax) > > movq %rdi, 2147450936(%rax) > > - movq 504(%rbx), %rax > > - movb $0, (%rax) > > + movb $0, 504(%rbx) > > jmp .L3 > > .L2: > > movq $0, 2147450880(%rax) > > > > There's missing one level of de-reference. Looking at clang: > > > > movq %rsi, 2147450928(%rax) > > movq %rdi, 2147450936(%rax) > > movq 504(%rbx), %rax > > movb $0, (%rax) > > jmp .L3 > > .L2: > > > > It does the same as my patch. > > Jakub? Even if so, just add that another level of indirection where it belongs, but as I said, what you posted didn't feel right. E.g. we just shouldn't reuse MEMs (even after adjusting them) from different indirection levels because we risk some attributes (alias set, MEM_EXPR, whatever else) will stay around from the different indirection level. Jakub
Re: [PATCH] Add missing store in emission of asan_stack_free.
On 6/1/20 3:18 PM, Martin Liška wrote: On 6/1/20 2:52 PM, Jakub Jelinek wrote: On Mon, Jun 01, 2020 at 02:28:51PM +0200, Martin Liška wrote: --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1598,8 +1598,24 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, if (use_after_return_class < 5 && can_store_by_pieces (sz, builtin_memset_read_str, , BITS_PER_UNIT, true)) - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , - BITS_PER_UNIT, true, RETURN_BEGIN); + { + /* Emit: + memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); + **SavedFlagPtr(FakeStack) = 0 + */ + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , + BITS_PER_UNIT, true, RETURN_BEGIN); + + unsigned HOST_WIDE_INT offset + = (1 << (use_after_return_class + 6)); + offset -= GET_MODE_SIZE (ptr_mode); + mem = adjust_address (mem, Pmode, offset); + mem = gen_rtx_MEM (ptr_mode, mem); + rtx tmp_reg = gen_reg_rtx (Pmode); + emit_move_insn (tmp_reg, mem); + mem = adjust_address (mem, QImode, 0); + emit_move_insn (mem, const0_rtx); This doesn't look correct to me. I'd think the first adjust_address should be mem = adjust_address (mem, ptr_mode, offset); which will give you a MEM with ptr_mode which has SavedFlagPtr(FakeStack) address, i.e. *SavedFlagPtr(FakeStack). Next, you want to load that into some temporary, so e.g. rtx addr = gen_reg_rtx (ptr_mode); emit_move_insn (addr, mem); next you need to convert that ptr_mode to Pmode if needed, so something like addr = convert_memory_address (Pmode, addr); and finally: mem = gen_rtx_MEM (QImode, addr); emit_move_insn (mem, const0_rtx); Completely untested. This is not correct. With your suggestion I have: int foo(int index) { int a[100]; return a[index]; } $ diff -u before.s after.s --- before.s 2020-06-01 15:15:22.634337654 +0200 +++ after.s 2020-06-01 15:16:32.205711511 +0200 @@ -81,8 +81,7 @@ movq %rdi, 2147450920(%rax) movq %rsi, 2147450928(%rax) movq %rdi, 2147450936(%rax) - movq 504(%rbx), %rax - movb $0, (%rax) + movb $0, 504(%rbx) jmp .L3 .L2: movq $0, 2147450880(%rax) There's missing one level of de-reference. Looking at clang: movq %rsi, 2147450928(%rax) movq %rdi, 2147450936(%rax) movq 504(%rbx), %rax movb $0, (%rax) jmp .L3 .L2: It does the same as my patch. Martin Jakub Jakub? Thanks, Martin
Re: [PATCH] Add missing store in emission of asan_stack_free.
On 6/1/20 2:52 PM, Jakub Jelinek wrote: On Mon, Jun 01, 2020 at 02:28:51PM +0200, Martin Liška wrote: --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1598,8 +1598,24 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, if (use_after_return_class < 5 && can_store_by_pieces (sz, builtin_memset_read_str, , BITS_PER_UNIT, true)) - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , -BITS_PER_UNIT, true, RETURN_BEGIN); + { + /* Emit: + memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); + **SavedFlagPtr(FakeStack) = 0 + */ + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , + BITS_PER_UNIT, true, RETURN_BEGIN); + + unsigned HOST_WIDE_INT offset + = (1 << (use_after_return_class + 6)); + offset -= GET_MODE_SIZE (ptr_mode); + mem = adjust_address (mem, Pmode, offset); + mem = gen_rtx_MEM (ptr_mode, mem); + rtx tmp_reg = gen_reg_rtx (Pmode); + emit_move_insn (tmp_reg, mem); + mem = adjust_address (mem, QImode, 0); + emit_move_insn (mem, const0_rtx); This doesn't look correct to me. I'd think the first adjust_address should be mem = adjust_address (mem, ptr_mode, offset); which will give you a MEM with ptr_mode which has SavedFlagPtr(FakeStack) address, i.e. *SavedFlagPtr(FakeStack). Next, you want to load that into some temporary, so e.g. rtx addr = gen_reg_rtx (ptr_mode); emit_move_insn (addr, mem); next you need to convert that ptr_mode to Pmode if needed, so something like addr = convert_memory_address (Pmode, addr); and finally: mem = gen_rtx_MEM (QImode, addr); emit_move_insn (mem, const0_rtx); Completely untested. This is not correct. With your suggestion I have: int foo(int index) { int a[100]; return a[index]; } $ diff -u before.s after.s --- before.s2020-06-01 15:15:22.634337654 +0200 +++ after.s 2020-06-01 15:16:32.205711511 +0200 @@ -81,8 +81,7 @@ movq%rdi, 2147450920(%rax) movq%rsi, 2147450928(%rax) movq%rdi, 2147450936(%rax) - movq504(%rbx), %rax - movb$0, (%rax) + movb$0, 504(%rbx) jmp .L3 .L2: movq$0, 2147450880(%rax) There's missing one level of de-reference. Looking at clang: movq%rsi, 2147450928(%rax) movq%rdi, 2147450936(%rax) movq504(%rbx), %rax movb$0, (%rax) jmp .L3 .L2: It does the same as my patch. Martin Jakub
Re: [PATCH] Add missing store in emission of asan_stack_free.
On Mon, Jun 01, 2020 at 02:28:51PM +0200, Martin Liška wrote: > --- a/gcc/asan.c > +++ b/gcc/asan.c > @@ -1598,8 +1598,24 @@ asan_emit_stack_protection (rtx base, rtx pbase, > unsigned int alignb, >if (use_after_return_class < 5 > && can_store_by_pieces (sz, builtin_memset_read_str, , > BITS_PER_UNIT, true)) > - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , > - BITS_PER_UNIT, true, RETURN_BEGIN); > + { > + /* Emit: > +memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); > +**SavedFlagPtr(FakeStack) = 0 > + */ > + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , > +BITS_PER_UNIT, true, RETURN_BEGIN); > + > + unsigned HOST_WIDE_INT offset > + = (1 << (use_after_return_class + 6)); > + offset -= GET_MODE_SIZE (ptr_mode); > + mem = adjust_address (mem, Pmode, offset); > + mem = gen_rtx_MEM (ptr_mode, mem); > + rtx tmp_reg = gen_reg_rtx (Pmode); > + emit_move_insn (tmp_reg, mem); > + mem = adjust_address (mem, QImode, 0); > + emit_move_insn (mem, const0_rtx); This doesn't look correct to me. I'd think the first adjust_address should be mem = adjust_address (mem, ptr_mode, offset); which will give you a MEM with ptr_mode which has SavedFlagPtr(FakeStack) address, i.e. *SavedFlagPtr(FakeStack). Next, you want to load that into some temporary, so e.g. rtx addr = gen_reg_rtx (ptr_mode); emit_move_insn (addr, mem); next you need to convert that ptr_mode to Pmode if needed, so something like addr = convert_memory_address (Pmode, addr); and finally: mem = gen_rtx_MEM (QImode, addr); emit_move_insn (mem, const0_rtx); Completely untested. Jakub
Re: [PATCH] Add missing store in emission of asan_stack_free.
On 5/20/20 1:03 PM, Franz Sirl wrote: Am 2020-05-19 um 21:05 schrieb Martin Liška: Hi. We make direct emission for asan_emit_stack_protection for smaller stacks. That's fine but we're missing the piece that marks the stack as released and we run out of pre-allocated stacks. I also included some stack-related constants that were used in asan.c. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: 2020-05-19 Martin Liska PR sanitizer/94910 * asan.c (asan_emit_stack_protection): Emit also **SavedFlagPtr(FakeStack) = 0 in order to release a stack frame. * asan.h (ASAN_MIN_STACK_FRAME_SIZE_LOG): New. (ASAN_MAX_STACK_FRAME_SIZE_LOG): Likewise. (ASAN_MIN_STACK_FRAME_SIZE): Likewise. (ASAN_MAX_STACK_FRAME_SIZE): Likewise. --- gcc/asan.c | 26 ++ gcc/asan.h | 8 2 files changed, 30 insertions(+), 4 deletions(-) >- if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase >+ if (asan_frame_size >= ASAN_MIN_STACK_FRAME_SIZE Hi, is the change from > to >= and from 32 to 64 for ASAN_MIN_STACK_FRAME_SIZE intentional? Just asking because it doesn't look obvious from Changelog or patch. Also a few lines below the "5" in use_after_return_class = floor_log2 (asan_frame_size - 1) - 5; looks like it may be related to ASAN_MIN_STACK_FRAME_SIZE_LOG. Hello. Thank you very much for the useful feedback. I really made the refactoring in a wrong way. I'm suggesting to only change the emission of asan_emit_stack_protection. Tested locally with asan.exp file. Ready for master? Thanks, Martin regards, Franz >From 5d0c64b2f4028af3ed575934ecc0c3378cca3de1 Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Tue, 19 May 2020 16:57:56 +0200 Subject: [PATCH] Add missing store in emission of asan_stack_free. gcc/ChangeLog: 2020-05-19 Martin Liska PR sanitizer/94910 * asan.c (asan_emit_stack_protection): Emit also **SavedFlagPtr(FakeStack) = 0 in order to release a stack frame. --- gcc/asan.c | 20 ++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/gcc/asan.c b/gcc/asan.c index c9872f1b007..e8d2a25ff79 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1598,8 +1598,24 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, if (use_after_return_class < 5 && can_store_by_pieces (sz, builtin_memset_read_str, , BITS_PER_UNIT, true)) - store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , - BITS_PER_UNIT, true, RETURN_BEGIN); + { + /* Emit: + memset(ShadowBase, kAsanStackAfterReturnMagic, ShadowSize); + **SavedFlagPtr(FakeStack) = 0 + */ + store_by_pieces (shadow_mem, sz, builtin_memset_read_str, , + BITS_PER_UNIT, true, RETURN_BEGIN); + + unsigned HOST_WIDE_INT offset + = (1 << (use_after_return_class + 6)); + offset -= GET_MODE_SIZE (ptr_mode); + mem = adjust_address (mem, Pmode, offset); + mem = gen_rtx_MEM (ptr_mode, mem); + rtx tmp_reg = gen_reg_rtx (Pmode); + emit_move_insn (tmp_reg, mem); + mem = adjust_address (mem, QImode, 0); + emit_move_insn (mem, const0_rtx); + } else if (use_after_return_class >= 5 || !set_storage_via_setmem (shadow_mem, GEN_INT (sz), -- 2.26.2
Re: [PATCH] Add missing store in emission of asan_stack_free.
Am 2020-05-19 um 21:05 schrieb Martin Liška: Hi. We make direct emission for asan_emit_stack_protection for smaller stacks. That's fine but we're missing the piece that marks the stack as released and we run out of pre-allocated stacks. I also included some stack-related constants that were used in asan.c. Patch can bootstrap on x86_64-linux-gnu and survives regression tests. Ready to be installed? Thanks, Martin gcc/ChangeLog: 2020-05-19 Martin Liska PR sanitizer/94910 * asan.c (asan_emit_stack_protection): Emit also **SavedFlagPtr(FakeStack) = 0 in order to release a stack frame. * asan.h (ASAN_MIN_STACK_FRAME_SIZE_LOG): New. (ASAN_MAX_STACK_FRAME_SIZE_LOG): Likewise. (ASAN_MIN_STACK_FRAME_SIZE): Likewise. (ASAN_MAX_STACK_FRAME_SIZE): Likewise. --- gcc/asan.c | 26 ++ gcc/asan.h | 8 2 files changed, 30 insertions(+), 4 deletions(-) >- if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase >+ if (asan_frame_size >= ASAN_MIN_STACK_FRAME_SIZE Hi, is the change from > to >= and from 32 to 64 for ASAN_MIN_STACK_FRAME_SIZE intentional? Just asking because it doesn't look obvious from Changelog or patch. Also a few lines below the "5" in use_after_return_class = floor_log2 (asan_frame_size - 1) - 5; looks like it may be related to ASAN_MIN_STACK_FRAME_SIZE_LOG. regards, Franz