* [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). @ 2017-06-27 13:05 Martin Liška 2017-06-27 15:29 ` Michael Matz 0 siblings, 1 reply; 13+ messages in thread From: Martin Liška @ 2017-06-27 13:05 UTC (permalink / raw) To: gcc-patches; +Cc: Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 3313 bytes --] Hi. Following bug was for me very educative. I learned that we support non-local gotos that can be combined with nested functions. Real fun :) Well, the problem is that both cfun->nonlocal_goto_save_area and cfun->static_chain_decl (emitted in expand_function_start) are put before NOTE_INSN_FUNCTION_BEG. And so result of expand_used_vars is put after these instrumentations. That causes problems as it uses stack before we initialize it (use-after-return checking): expanded cfun->static_chain_decl: (note 1 0 5 NOTE_INSN_DELETED) (note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) (insn 2 5 3 2 (set (reg/f:DI 88 [ CHAIN.1 ]) (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 (nil)) (insn 3 2 4 2 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) (const_int -8 [0xfffffffffffffff8])) [0 S8 A64]) (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 (nil)) (note 4 3 7 2 NOTE_INSN_FUNCTION_BEG) (insn 7 4 8 2 (set (reg/f:DI 87 [ _2 ]) (reg/f:DI 88 [ CHAIN.1 ])) "pr81186.c":5 -1 (nil)) (call_insn 8 7 9 2 (call (mem:QI (symbol_ref:DI ("__asan_handle_no_return") [flags 0x41] <function_decl 0x2ba005ad5d00 __builtin___asan_handle_no_return>) [0 __builtin___asan_handle_no_return S1 A8]) (const_int 0 [0])) "pr81186.c":5 -1 (expr_list:REG_EH_REGION (const_int 0 [0]) (nil)) (nil)) expanded cfun->nonlocal_goto_save_area: (note 1 0 34 NOTE_INSN_DELETED) (note 34 1 31 2 [bb 2] NOTE_INSN_BASIC_BLOCK) (insn 31 34 32 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) (const_int -64 [0xffffffffffffffc0])) [4 FRAME.0.__nl_goto_buf+0 S8 A64]) (reg/f:DI 82 virtual-stack-vars)) "pr81186.c":3 -1 (nil)) (insn 32 31 2 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) (const_int -56 [0xffffffffffffffc8])) [4 FRAME.0.__nl_goto_buf+8 S8 A64]) (reg/f:DI 7 sp)) "pr81186.c":3 -1 (nil)) (insn 2 32 3 2 (parallel [ (set (reg:DI 96) (plus:DI (reg/f:DI 82 virtual-stack-vars) (const_int -96 [0xffffffffffffffa0]))) (clobber (reg:CC 17 flags)) ]) "pr81186.c":3 -1 (nil)) (insn 3 2 4 2 (set (reg:DI 97) (reg:DI 96)) "pr81186.c":3 -1 (nil)) (insn 4 3 5 2 (set (reg:CCZ 17 flags) (compare:CCZ (mem/c:SI (symbol_ref:DI ("__asan_option_detect_stack_use_after_return") [flags 0x40] <var_decl 0x2ba005b5b750 __asan_option_detect_stack_use_after_return>) [5 __asan_option_detect_stack_use_after_return+0 S4 A32]) (const_int 0 [0]))) "pr81186.c":3 -1 (nil)) And thus I suggest to move both these instrumentations after NOTE_INSN_FUNCTION_BEG. Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Ready to be installed? Martin gcc/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * function.c (expand_function_start): Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG. gcc/testsuite/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * gcc.dg/asan/pr81186.c: New test. --- gcc/function.c | 18 +++++++++--------- gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++ 2 files changed, 22 insertions(+), 9 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c [-- Attachment #2: 0001-Move-static-chain-and-non-local-goto-init-after-NOTE.patch --] [-- Type: text/x-patch, Size: 1513 bytes --] diff --git a/gcc/function.c b/gcc/function.c index f625489205b..5e8a56099a5 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -5220,6 +5220,14 @@ expand_function_start (tree subr) In some cases this requires emitting insns. */ assign_parms (subr); + /* The following was moved from init_function_start. + The move is supposed to make sdb output more accurate. */ + /* Indicate the beginning of the function body, + as opposed to parm setup. */ + rtx_note *b = emit_note (NOTE_INSN_FUNCTION_BEG); + + gcc_assert (NOTE_P (get_last_insn ())); + /* If function gets a static chain arg, store it. */ if (cfun->static_chain_decl) { @@ -5284,15 +5292,7 @@ expand_function_start (tree subr) update_nonlocal_goto_save_area (); } - /* The following was moved from init_function_start. - The move is supposed to make sdb output more accurate. */ - /* Indicate the beginning of the function body, - as opposed to parm setup. */ - emit_note (NOTE_INSN_FUNCTION_BEG); - - gcc_assert (NOTE_P (get_last_insn ())); - - parm_birth_insn = get_last_insn (); + parm_birth_insn = b; if (crtl->profile) { diff --git a/gcc/testsuite/gcc.dg/asan/pr81186.c b/gcc/testsuite/gcc.dg/asan/pr81186.c new file mode 100644 index 00000000000..74d3837a482 --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/pr81186.c @@ -0,0 +1,13 @@ +/* PR sanitizer/81186 */ +/* { dg-do run } */ + +int +main () +{ + __label__ l; + void f () { goto l; } + + f (); +l: + return 0; +} ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-06-27 13:05 [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186) Martin Liška @ 2017-06-27 15:29 ` Michael Matz 2017-06-28 11:11 ` Martin Liška 0 siblings, 1 reply; 13+ messages in thread From: Michael Matz @ 2017-06-27 15:29 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 4654 bytes --] Hi, On Tue, 27 Jun 2017, Martin LiÅ¡ka wrote: > Following bug was for me very educative. I learned that we support > non-local gotos that can be combined with nested functions. Real fun :) > Well, the problem is that both cfun->nonlocal_goto_save_area and > cfun->static_chain_decl (emitted in expand_function_start) are put > before NOTE_INSN_FUNCTION_BEG. And so result of expand_used_vars is put > after these instrumentations. That causes problems as it uses stack > before we initialize it (use-after-return checking): I don't think that's the right fix. The purpose of NOTE_INSN_FUNCTION_BEG is to mark the "real" beginning of the function, i.e. without all the compiler generated stuff that's necessary to set up parameters or local variables and so on. The goto save area and the static chain are also such compiler generated implementation details, and hence are correctly put in front of the function begin note. Also if you put something in front of the static_chain_decl initialization (as you do if you move the parm_birth_insn in front of it) you'd have to make sure that the incoming hidding parameter containing the static chain (r10 on x86_64) isn't clobbered from function start up to that point. So that won't work either generally. I don't know what exactly is the issue with calling __asan_handle_no_return before the other instructions emitted by expand_used_vars. Either it shouldn't be called for the static chain (i.e. not instrumented) or whatever setup asan needs needs to happen in front of the static chain setup, but then without clobbering the incoming static chain param (!). Ciao, Michael. > > expanded cfun->static_chain_decl: > > (note 1 0 5 NOTE_INSN_DELETED) > (note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) > (insn 2 5 3 2 (set (reg/f:DI 88 [ CHAIN.1 ]) > (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 > (nil)) > (insn 3 2 4 2 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) > (const_int -8 [0xfffffffffffffff8])) [0 S8 A64]) > (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 > (nil)) > (note 4 3 7 2 NOTE_INSN_FUNCTION_BEG) > (insn 7 4 8 2 (set (reg/f:DI 87 [ _2 ]) > (reg/f:DI 88 [ CHAIN.1 ])) "pr81186.c":5 -1 > (nil)) > (call_insn 8 7 9 2 (call (mem:QI (symbol_ref:DI ("__asan_handle_no_return") [flags 0x41] <function_decl 0x2ba005ad5d00 __builtin___asan_handle_no_return>) [0 __builtin___asan_handle_no_return S1 A8]) > (const_int 0 [0])) "pr81186.c":5 -1 > (expr_list:REG_EH_REGION (const_int 0 [0]) > (nil)) > (nil)) > > expanded cfun->nonlocal_goto_save_area: > > (note 1 0 34 NOTE_INSN_DELETED) > (note 34 1 31 2 [bb 2] NOTE_INSN_BASIC_BLOCK) > (insn 31 34 32 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) > (const_int -64 [0xffffffffffffffc0])) [4 FRAME.0.__nl_goto_buf+0 S8 A64]) > (reg/f:DI 82 virtual-stack-vars)) "pr81186.c":3 -1 > (nil)) > (insn 32 31 2 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) > (const_int -56 [0xffffffffffffffc8])) [4 FRAME.0.__nl_goto_buf+8 S8 A64]) > (reg/f:DI 7 sp)) "pr81186.c":3 -1 > (nil)) > (insn 2 32 3 2 (parallel [ > (set (reg:DI 96) > (plus:DI (reg/f:DI 82 virtual-stack-vars) > (const_int -96 [0xffffffffffffffa0]))) > (clobber (reg:CC 17 flags)) > ]) "pr81186.c":3 -1 > (nil)) > (insn 3 2 4 2 (set (reg:DI 97) > (reg:DI 96)) "pr81186.c":3 -1 > (nil)) > (insn 4 3 5 2 (set (reg:CCZ 17 flags) > (compare:CCZ (mem/c:SI (symbol_ref:DI ("__asan_option_detect_stack_use_after_return") [flags 0x40] <var_decl 0x2ba005b5b750 __asan_option_detect_stack_use_after_return>) [5 __asan_option_detect_stack_use_after_return+0 S4 A32]) > (const_int 0 [0]))) "pr81186.c":3 -1 > (nil)) > > And thus I suggest to move both these instrumentations after NOTE_INSN_FUNCTION_BEG. > Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > > Ready to be installed? > Martin > > gcc/ChangeLog: > > 2017-06-27 Martin Liska <mliska@suse.cz> > > PR sanitize/81186 > * function.c (expand_function_start): Move static chain and non-local > goto init after NOTE_INSN_FUNCTION_BEG. > > gcc/testsuite/ChangeLog: > > 2017-06-27 Martin Liska <mliska@suse.cz> > > PR sanitize/81186 > * gcc.dg/asan/pr81186.c: New test. > --- > gcc/function.c | 18 +++++++++--------- > gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++ > 2 files changed, 22 insertions(+), 9 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-06-27 15:29 ` Michael Matz @ 2017-06-28 11:11 ` Martin Liška 2017-06-30 14:03 ` Michael Matz 0 siblings, 1 reply; 13+ messages in thread From: Martin Liška @ 2017-06-28 11:11 UTC (permalink / raw) To: Michael Matz; +Cc: gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 5380 bytes --] On 06/27/2017 05:29 PM, Michael Matz wrote: > Hi, > > On Tue, 27 Jun 2017, Martin LiÅ¡ka wrote: > >> Following bug was for me very educative. I learned that we support >> non-local gotos that can be combined with nested functions. Real fun :) >> Well, the problem is that both cfun->nonlocal_goto_save_area and >> cfun->static_chain_decl (emitted in expand_function_start) are put >> before NOTE_INSN_FUNCTION_BEG. And so result of expand_used_vars is put >> after these instrumentations. That causes problems as it uses stack >> before we initialize it (use-after-return checking): > > I don't think that's the right fix. The purpose of > NOTE_INSN_FUNCTION_BEG is to mark the "real" beginning of the function, > i.e. without all the compiler generated stuff that's necessary to set up > parameters or local variables and so on. The goto save area and the > static chain are also such compiler generated implementation details, and > hence are correctly put in front of the function begin note. Hello. Thanks for the review. I'm not so familiar with RTL, but hopefully new version of the patch should do it properly. Idea is to come up with a new asan_stack_birth_insn that points after place where stack is ready to use (when one uses use-after-return). And then in function.c static_chain_decl and nonlocal_goto_save_area expansion is generated to a new sequence and the sequence is put after the asan_stack_birth_insn. > > Also if you put something in front of the static_chain_decl initialization > (as you do if you move the parm_birth_insn in front of it) you'd have to > make sure that the incoming hidding parameter containing the static chain > (r10 on x86_64) isn't clobbered from function start up to that point. > So that won't work either generally. > > I don't know what exactly is the issue with calling > __asan_handle_no_return before the other instructions emitted by > expand_used_vars. Either it shouldn't be called for the static chain > (i.e. not instrumented) or whatever setup asan needs needs to happen in > front of the static chain setup, but then without clobbering the incoming > static chain param (!). Here I need to have FRAME variable to be sanitized as seen in pr78541.c and pr78541-2.c test-cases. Thoughts? Thanks, Martin > > > Ciao, > Michael. >> >> expanded cfun->static_chain_decl: >> >> (note 1 0 5 NOTE_INSN_DELETED) >> (note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) >> (insn 2 5 3 2 (set (reg/f:DI 88 [ CHAIN.1 ]) >> (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 >> (nil)) >> (insn 3 2 4 2 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) >> (const_int -8 [0xfffffffffffffff8])) [0 S8 A64]) >> (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 >> (nil)) >> (note 4 3 7 2 NOTE_INSN_FUNCTION_BEG) >> (insn 7 4 8 2 (set (reg/f:DI 87 [ _2 ]) >> (reg/f:DI 88 [ CHAIN.1 ])) "pr81186.c":5 -1 >> (nil)) >> (call_insn 8 7 9 2 (call (mem:QI (symbol_ref:DI ("__asan_handle_no_return") [flags 0x41] <function_decl 0x2ba005ad5d00 __builtin___asan_handle_no_return>) [0 __builtin___asan_handle_no_return S1 A8]) >> (const_int 0 [0])) "pr81186.c":5 -1 >> (expr_list:REG_EH_REGION (const_int 0 [0]) >> (nil)) >> (nil)) >> >> expanded cfun->nonlocal_goto_save_area: >> >> (note 1 0 34 NOTE_INSN_DELETED) >> (note 34 1 31 2 [bb 2] NOTE_INSN_BASIC_BLOCK) >> (insn 31 34 32 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) >> (const_int -64 [0xffffffffffffffc0])) [4 FRAME.0.__nl_goto_buf+0 S8 A64]) >> (reg/f:DI 82 virtual-stack-vars)) "pr81186.c":3 -1 >> (nil)) >> (insn 32 31 2 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) >> (const_int -56 [0xffffffffffffffc8])) [4 FRAME.0.__nl_goto_buf+8 S8 A64]) >> (reg/f:DI 7 sp)) "pr81186.c":3 -1 >> (nil)) >> (insn 2 32 3 2 (parallel [ >> (set (reg:DI 96) >> (plus:DI (reg/f:DI 82 virtual-stack-vars) >> (const_int -96 [0xffffffffffffffa0]))) >> (clobber (reg:CC 17 flags)) >> ]) "pr81186.c":3 -1 >> (nil)) >> (insn 3 2 4 2 (set (reg:DI 97) >> (reg:DI 96)) "pr81186.c":3 -1 >> (nil)) >> (insn 4 3 5 2 (set (reg:CCZ 17 flags) >> (compare:CCZ (mem/c:SI (symbol_ref:DI ("__asan_option_detect_stack_use_after_return") [flags 0x40] <var_decl 0x2ba005b5b750 __asan_option_detect_stack_use_after_return>) [5 __asan_option_detect_stack_use_after_return+0 S4 A32]) >> (const_int 0 [0]))) "pr81186.c":3 -1 >> (nil)) >> >> And thus I suggest to move both these instrumentations after NOTE_INSN_FUNCTION_BEG. >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >> >> Ready to be installed? >> Martin >> >> gcc/ChangeLog: >> >> 2017-06-27 Martin Liska <mliska@suse.cz> >> >> PR sanitize/81186 >> * function.c (expand_function_start): Move static chain and non-local >> goto init after NOTE_INSN_FUNCTION_BEG. >> >> gcc/testsuite/ChangeLog: >> >> 2017-06-27 Martin Liska <mliska@suse.cz> >> >> PR sanitize/81186 >> * gcc.dg/asan/pr81186.c: New test. >> --- >> gcc/function.c | 18 +++++++++--------- >> gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++ >> 2 files changed, 22 insertions(+), 9 deletions(-) >> create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c >> >> [-- Attachment #2: 0001-Emit-nonlocal_goto_save_area-and-static_chain_decl-p.patch --] [-- Type: text/x-patch, Size: 3648 bytes --] From d52e3359b189aaa17fd8cd42a7e1ea72227a452d Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Fri, 23 Jun 2017 14:05:59 +0200 Subject: [PATCH] Emit nonlocal_goto_save_area and static_chain_decl properly (PR sanitize/81186). gcc/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * asan.c (asan_emit_stack_protection): Emit asan_stack_birth_insn. * emit-rtl.h (struct GTY): Add x_asan_stack_birth_insn. * function.c (expand_function_start): Move nonlocal_goto_save_area and static_chain_decl expansion after asan_stack_birth_insn if set. gcc/testsuite/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * gcc.dg/asan/pr81186.c: New test. --- gcc/asan.c | 1 + gcc/emit-rtl.h | 4 ++++ gcc/function.c | 13 +++++++++++++ gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++ 4 files changed, 31 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c diff --git a/gcc/asan.c b/gcc/asan.c index e730530930b..c7c839fb1a1 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1168,6 +1168,7 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, gen_int_mode (base_align_bias - base_offset, Pmode), NULL_RTX, 1, OPTAB_DIRECT)); + asan_stack_birth_insn = get_last_insn (); } mem = gen_rtx_MEM (ptr_mode, base); mem = adjust_address (mem, VOIDmode, base_align_bias); diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h index b455b4c00fb..7658fa121f2 100644 --- a/gcc/emit-rtl.h +++ b/gcc/emit-rtl.h @@ -128,6 +128,9 @@ struct GTY(()) rtl_data { If stack grows up, this is the address for the next slot. */ HOST_WIDE_INT x_frame_offset; + /* Insn after which AddressSanitizer prepares stack for use. */ + rtx_insn *x_asan_stack_birth_insn; + /* Insn after which register parms and SAVE_EXPRs are born, if nonopt. */ rtx_insn *x_parm_birth_insn; @@ -298,6 +301,7 @@ struct GTY(()) rtl_data { #define return_label (crtl->x_return_label) #define naked_return_label (crtl->x_naked_return_label) #define stack_slot_list (crtl->x_stack_slot_list) +#define asan_stack_birth_insn (crtl->x_asan_stack_birth_insn) #define parm_birth_insn (crtl->x_parm_birth_insn) #define frame_offset (crtl->x_frame_offset) #define stack_check_probe_note (crtl->x_stack_check_probe_note) diff --git a/gcc/function.c b/gcc/function.c index f625489205b..c0c626d32c4 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -5220,6 +5220,8 @@ expand_function_start (tree subr) In some cases this requires emitting insns. */ assign_parms (subr); + start_sequence (); + /* If function gets a static chain arg, store it. */ if (cfun->static_chain_decl) { @@ -5284,6 +5286,17 @@ expand_function_start (tree subr) update_nonlocal_goto_save_area (); } + rtx_insn *seq = get_insns (); + end_sequence (); + + if (seq) + { + if (asan_stack_birth_insn == NULL_RTX) + asan_stack_birth_insn = get_insns (); + + emit_insn_after (seq, asan_stack_birth_insn); + } + /* The following was moved from init_function_start. The move is supposed to make sdb output more accurate. */ /* Indicate the beginning of the function body, diff --git a/gcc/testsuite/gcc.dg/asan/pr81186.c b/gcc/testsuite/gcc.dg/asan/pr81186.c new file mode 100644 index 00000000000..74d3837a482 --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/pr81186.c @@ -0,0 +1,13 @@ +/* PR sanitizer/81186 */ +/* { dg-do run } */ + +int +main () +{ + __label__ l; + void f () { goto l; } + + f (); +l: + return 0; +} -- 2.13.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-06-28 11:11 ` Martin Liška @ 2017-06-30 14:03 ` Michael Matz 2017-06-30 14:33 ` Martin Liška 2017-07-13 14:15 ` Martin Liška 0 siblings, 2 replies; 13+ messages in thread From: Michael Matz @ 2017-06-30 14:03 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 6140 bytes --] Hi, On Wed, 28 Jun 2017, Martin Liška wrote: > Thanks for the review. I'm not so familiar with RTL, but hopefully new > version of the patch should do it properly. Idea is to come up with a > new asan_stack_birth_insn that points after place where stack is ready > to use (when one uses use-after-return). And then in function.c > static_chain_decl and nonlocal_goto_save_area expansion is generated to > a new sequence and the sequence is put after the asan_stack_birth_insn. That has the same problem. The code for function start is conceptually like this: entry_point: setup return space: on some targets the address of the return buffer is passed in a certain incoming arg (i.e. it's like an argument) setup arguments: store away incoming registers into pseudo load stack slot arguments (or put them there, all target dependend) * point 1 where you insert code * setup static chain: conceptually this is just a hidden additional function argument setup nonlocal goto save area If you simply create any other code inside this sequence you potentially have the problem of clobbering any of the incoming hardregs. In simple examples you might not notice when the register allocator heeds the hardreg uses, but if you for instance call other functions in there you most likely clobber some. E.g. r10 (the incoming static chain pointer on x86-64) isn't preserved across function calls. So if you call a function at point 1 above you clobber r10, but the code for setting up the static chain needs it as input. So you need to find some other solution of setting up the stack for ASAN. And it'd be best if that solution doesn't require inserting code inside the above sequence of parameter setup instructions, and you certainly can't call any functions inside that sequence. It might mean that you can't track the static chain place or the nonlocal goto save area. You also don't track the parameter stack slots, right? Ciao, Michael. > > > > > Also if you put something in front of the static_chain_decl initialization > > (as you do if you move the parm_birth_insn in front of it) you'd have to > > make sure that the incoming hidding parameter containing the static chain > > (r10 on x86_64) isn't clobbered from function start up to that point. > > So that won't work either generally. > > > > I don't know what exactly is the issue with calling > > __asan_handle_no_return before the other instructions emitted by > > expand_used_vars. Either it shouldn't be called for the static chain > > (i.e. not instrumented) or whatever setup asan needs needs to happen in > > front of the static chain setup, but then without clobbering the incoming > > static chain param (!). > > Here I need to have FRAME variable to be sanitized as seen in pr78541.c > and pr78541-2.c test-cases. > > Thoughts? > Thanks, > Martin > > > > > > > Ciao, > > Michael. > >> > >> expanded cfun->static_chain_decl: > >> > >> (note 1 0 5 NOTE_INSN_DELETED) > >> (note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) > >> (insn 2 5 3 2 (set (reg/f:DI 88 [ CHAIN.1 ]) > >> (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 > >> (nil)) > >> (insn 3 2 4 2 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) > >> (const_int -8 [0xfffffffffffffff8])) [0 S8 A64]) > >> (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 > >> (nil)) > >> (note 4 3 7 2 NOTE_INSN_FUNCTION_BEG) > >> (insn 7 4 8 2 (set (reg/f:DI 87 [ _2 ]) > >> (reg/f:DI 88 [ CHAIN.1 ])) "pr81186.c":5 -1 > >> (nil)) > >> (call_insn 8 7 9 2 (call (mem:QI (symbol_ref:DI ("__asan_handle_no_return") [flags 0x41] <function_decl 0x2ba005ad5d00 __builtin___asan_handle_no_return>) [0 __builtin___asan_handle_no_return S1 A8]) > >> (const_int 0 [0])) "pr81186.c":5 -1 > >> (expr_list:REG_EH_REGION (const_int 0 [0]) > >> (nil)) > >> (nil)) > >> > >> expanded cfun->nonlocal_goto_save_area: > >> > >> (note 1 0 34 NOTE_INSN_DELETED) > >> (note 34 1 31 2 [bb 2] NOTE_INSN_BASIC_BLOCK) > >> (insn 31 34 32 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) > >> (const_int -64 [0xffffffffffffffc0])) [4 FRAME.0.__nl_goto_buf+0 S8 A64]) > >> (reg/f:DI 82 virtual-stack-vars)) "pr81186.c":3 -1 > >> (nil)) > >> (insn 32 31 2 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) > >> (const_int -56 [0xffffffffffffffc8])) [4 FRAME.0.__nl_goto_buf+8 S8 A64]) > >> (reg/f:DI 7 sp)) "pr81186.c":3 -1 > >> (nil)) > >> (insn 2 32 3 2 (parallel [ > >> (set (reg:DI 96) > >> (plus:DI (reg/f:DI 82 virtual-stack-vars) > >> (const_int -96 [0xffffffffffffffa0]))) > >> (clobber (reg:CC 17 flags)) > >> ]) "pr81186.c":3 -1 > >> (nil)) > >> (insn 3 2 4 2 (set (reg:DI 97) > >> (reg:DI 96)) "pr81186.c":3 -1 > >> (nil)) > >> (insn 4 3 5 2 (set (reg:CCZ 17 flags) > >> (compare:CCZ (mem/c:SI (symbol_ref:DI ("__asan_option_detect_stack_use_after_return") [flags 0x40] <var_decl 0x2ba005b5b750 __asan_option_detect_stack_use_after_return>) [5 __asan_option_detect_stack_use_after_return+0 S4 A32]) > >> (const_int 0 [0]))) "pr81186.c":3 -1 > >> (nil)) > >> > >> And thus I suggest to move both these instrumentations after NOTE_INSN_FUNCTION_BEG. > >> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. > >> > >> Ready to be installed? > >> Martin > >> > >> gcc/ChangeLog: > >> > >> 2017-06-27 Martin Liska <mliska@suse.cz> > >> > >> PR sanitize/81186 > >> * function.c (expand_function_start): Move static chain and non-local > >> goto init after NOTE_INSN_FUNCTION_BEG. > >> > >> gcc/testsuite/ChangeLog: > >> > >> 2017-06-27 Martin Liska <mliska@suse.cz> > >> > >> PR sanitize/81186 > >> * gcc.dg/asan/pr81186.c: New test. > >> --- > >> gcc/function.c | 18 +++++++++--------- > >> gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++ > >> 2 files changed, 22 insertions(+), 9 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c > >> > >> > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-06-30 14:03 ` Michael Matz @ 2017-06-30 14:33 ` Martin Liška 2017-07-13 14:15 ` Martin Liška 1 sibling, 0 replies; 13+ messages in thread From: Martin Liška @ 2017-06-30 14:33 UTC (permalink / raw) To: Michael Matz; +Cc: gcc-patches, Jakub Jelinek On 06/30/2017 04:03 PM, Michael Matz wrote: > Hi, > > On Wed, 28 Jun 2017, Martin Liška wrote: > >> Thanks for the review. I'm not so familiar with RTL, but hopefully new >> version of the patch should do it properly. Idea is to come up with a >> new asan_stack_birth_insn that points after place where stack is ready >> to use (when one uses use-after-return). And then in function.c >> static_chain_decl and nonlocal_goto_save_area expansion is generated to >> a new sequence and the sequence is put after the asan_stack_birth_insn. > > That has the same problem. > > The code for function start is conceptually like this: > > entry_point: > setup return space: > on some targets the address of the return buffer is passed in a > certain incoming arg (i.e. it's like an argument) > setup arguments: > store away incoming registers into pseudo > load stack slot arguments (or put them there, all target dependend) > * point 1 where you insert code * > setup static chain: > conceptually this is just a hidden additional function argument > setup nonlocal goto save area > > If you simply create any other code inside this sequence you potentially > have the problem of clobbering any of the incoming hardregs. In simple > examples you might not notice when the register allocator heeds the > hardreg uses, but if you for instance call other functions in there you > most likely clobber some. E.g. r10 (the incoming static chain pointer on > x86-64) isn't preserved across function calls. So if you call a function > at point 1 above you clobber r10, but the code for setting up the static > chain needs it as input. Thanks Michael. Now I got it as I understand the problematic of usage of r10 register. Actually it's easy to come up with a test-case that breaks that: $ cat /home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c /* PR sanitizer/81186 */ /* { dg-do run } */ int main () { __label__ l; void f () { int a[123]; goto l; } f (); l: return 0; } where: f.2139: .LASANPC1: .LFB1: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset 6, -16 movq %rsp, %rbp .cfi_def_cfa_register 6 pushq %rbx subq $600, %rsp .cfi_offset 3, -24 leaq -592(%rbp), %rbx cmpl $0, __asan_option_detect_stack_use_after_return(%rip) je .L1 movl $576, %edi call __asan_stack_malloc_4 <--- here clobbering of r10 happens testq %rax, %rax je .L1 movq %rax, %rbx .L1: movq %r10, %rdx <- use after it's clobbered movq %r10, -600(%rbp) movq $1102416563, (%rbx) > > So you need to find some other solution of setting up the stack for ASAN. > And it'd be best if that solution doesn't require inserting code inside > the above sequence of parameter setup instructions, and you certainly > can't call any functions inside that sequence. It might mean that you > can't track the static chain place or the nonlocal goto save area. You > also don't track the parameter stack slots, right? IIUC parameter stack slots are not sanitized. I will think about it more ;) Martin > > > Ciao, > Michael. >> >>> >>> Also if you put something in front of the static_chain_decl initialization >>> (as you do if you move the parm_birth_insn in front of it) you'd have to >>> make sure that the incoming hidding parameter containing the static chain >>> (r10 on x86_64) isn't clobbered from function start up to that point. >>> So that won't work either generally. >>> >>> I don't know what exactly is the issue with calling >>> __asan_handle_no_return before the other instructions emitted by >>> expand_used_vars. Either it shouldn't be called for the static chain >>> (i.e. not instrumented) or whatever setup asan needs needs to happen in >>> front of the static chain setup, but then without clobbering the incoming >>> static chain param (!). >> >> Here I need to have FRAME variable to be sanitized as seen in pr78541.c >> and pr78541-2.c test-cases. >> >> Thoughts? >> Thanks, >> Martin >> >>> >>> >>> Ciao, >>> Michael. >>>> >>>> expanded cfun->static_chain_decl: >>>> >>>> (note 1 0 5 NOTE_INSN_DELETED) >>>> (note 5 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) >>>> (insn 2 5 3 2 (set (reg/f:DI 88 [ CHAIN.1 ]) >>>> (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 >>>> (nil)) >>>> (insn 3 2 4 2 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) >>>> (const_int -8 [0xfffffffffffffff8])) [0 S8 A64]) >>>> (reg:DI 39 r10 [ CHAIN.1 ])) "pr81186.c":5 -1 >>>> (nil)) >>>> (note 4 3 7 2 NOTE_INSN_FUNCTION_BEG) >>>> (insn 7 4 8 2 (set (reg/f:DI 87 [ _2 ]) >>>> (reg/f:DI 88 [ CHAIN.1 ])) "pr81186.c":5 -1 >>>> (nil)) >>>> (call_insn 8 7 9 2 (call (mem:QI (symbol_ref:DI ("__asan_handle_no_return") [flags 0x41] <function_decl 0x2ba005ad5d00 __builtin___asan_handle_no_return>) [0 __builtin___asan_handle_no_return S1 A8]) >>>> (const_int 0 [0])) "pr81186.c":5 -1 >>>> (expr_list:REG_EH_REGION (const_int 0 [0]) >>>> (nil)) >>>> (nil)) >>>> >>>> expanded cfun->nonlocal_goto_save_area: >>>> >>>> (note 1 0 34 NOTE_INSN_DELETED) >>>> (note 34 1 31 2 [bb 2] NOTE_INSN_BASIC_BLOCK) >>>> (insn 31 34 32 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) >>>> (const_int -64 [0xffffffffffffffc0])) [4 FRAME.0.__nl_goto_buf+0 S8 A64]) >>>> (reg/f:DI 82 virtual-stack-vars)) "pr81186.c":3 -1 >>>> (nil)) >>>> (insn 32 31 2 2 (set (mem/f/c:DI (plus:DI (reg:DI 95) >>>> (const_int -56 [0xffffffffffffffc8])) [4 FRAME.0.__nl_goto_buf+8 S8 A64]) >>>> (reg/f:DI 7 sp)) "pr81186.c":3 -1 >>>> (nil)) >>>> (insn 2 32 3 2 (parallel [ >>>> (set (reg:DI 96) >>>> (plus:DI (reg/f:DI 82 virtual-stack-vars) >>>> (const_int -96 [0xffffffffffffffa0]))) >>>> (clobber (reg:CC 17 flags)) >>>> ]) "pr81186.c":3 -1 >>>> (nil)) >>>> (insn 3 2 4 2 (set (reg:DI 97) >>>> (reg:DI 96)) "pr81186.c":3 -1 >>>> (nil)) >>>> (insn 4 3 5 2 (set (reg:CCZ 17 flags) >>>> (compare:CCZ (mem/c:SI (symbol_ref:DI ("__asan_option_detect_stack_use_after_return") [flags 0x40] <var_decl 0x2ba005b5b750 __asan_option_detect_stack_use_after_return>) [5 __asan_option_detect_stack_use_after_return+0 S4 A32]) >>>> (const_int 0 [0]))) "pr81186.c":3 -1 >>>> (nil)) >>>> >>>> And thus I suggest to move both these instrumentations after NOTE_INSN_FUNCTION_BEG. >>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>> >>>> Ready to be installed? >>>> Martin >>>> >>>> gcc/ChangeLog: >>>> >>>> 2017-06-27 Martin Liska <mliska@suse.cz> >>>> >>>> PR sanitize/81186 >>>> * function.c (expand_function_start): Move static chain and non-local >>>> goto init after NOTE_INSN_FUNCTION_BEG. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> 2017-06-27 Martin Liska <mliska@suse.cz> >>>> >>>> PR sanitize/81186 >>>> * gcc.dg/asan/pr81186.c: New test. >>>> --- >>>> gcc/function.c | 18 +++++++++--------- >>>> gcc/testsuite/gcc.dg/asan/pr81186.c | 13 +++++++++++++ >>>> 2 files changed, 22 insertions(+), 9 deletions(-) >>>> create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c >>>> >>>> >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-06-30 14:03 ` Michael Matz 2017-06-30 14:33 ` Martin Liška @ 2017-07-13 14:15 ` Martin Liška 2017-07-14 13:42 ` Michael Matz 1 sibling, 1 reply; 13+ messages in thread From: Martin Liška @ 2017-07-13 14:15 UTC (permalink / raw) To: Michael Matz; +Cc: gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 720 bytes --] On 06/30/2017 04:03 PM, Michael Matz wrote: > So you need to find some other solution of setting up the stack for ASAN. > And it'd be best if that solution doesn't require inserting code inside > the above sequence of parameter setup instructions, and you certainly > can't call any functions inside that sequence. It might mean that you > can't track the static chain place or the nonlocal goto save area. You > also don't track the parameter stack slots, right? Hi. Hopefully following patch will fix that. I returned to the first version and saved/restored static_chain register before/after __asan_stack_malloc. Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. Thoughts? Martin [-- Attachment #2: 0001-Move-static-chain-and-non-local-goto-init-after-NOTE-v3.patch --] [-- Type: text/x-patch, Size: 3789 bytes --] From b285e7cb1d7f3e35981dec951121db58ce152b3b Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Thu, 13 Jul 2017 13:37:47 +0200 Subject: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG gcc/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * function.c (expand_function_start): Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG. * asan.c (asan_emit_stack_protection): Preserve static chain register if we call __asan_stack_malloc_N. gcc/testsuite/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * gcc.dg/asan/pr81186.c: New test. --- gcc/asan.c | 12 ++++++++++++ gcc/function.c | 18 +++++++++--------- gcc/testsuite/gcc.dg/asan/pr81186.c | 18 ++++++++++++++++++ 3 files changed, 39 insertions(+), 9 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c diff --git a/gcc/asan.c b/gcc/asan.c index 89c2731e8cd..9cc1d21c1fb 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1340,6 +1340,16 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, emit_cmp_and_jump_insns (ret, const0_rtx, EQ, NULL_RTX, VOIDmode, 0, lab, profile_probability::very_likely ()); + /* Preserve static chain register in order to not have it clobbered in + __asan_stack_malloc_N function. */ + rtx chain = targetm.calls.static_chain (current_function_decl, true); + rtx saved_chain; + if (chain) + { + saved_chain = gen_reg_rtx (Pmode); + emit_move_insn (saved_chain, chain); + } + snprintf (buf, sizeof buf, "__asan_stack_malloc_%d", use_after_return_class); ret = init_one_libfunc (buf); @@ -1347,6 +1357,8 @@ asan_emit_stack_protection (rtx base, rtx pbase, unsigned int alignb, GEN_INT (asan_frame_size + base_align_bias), TYPE_MODE (pointer_sized_int_node)); + if (chain) + emit_move_insn (chain, saved_chain); /* __asan_stack_malloc_[n] returns a pointer to fake stack if succeeded and NULL otherwise. Check RET value is NULL here and jump over the BASE reassignment in this case. Otherwise, reassign BASE to RET. */ diff --git a/gcc/function.c b/gcc/function.c index f625489205b..5e8a56099a5 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -5220,6 +5220,14 @@ expand_function_start (tree subr) In some cases this requires emitting insns. */ assign_parms (subr); + /* The following was moved from init_function_start. + The move is supposed to make sdb output more accurate. */ + /* Indicate the beginning of the function body, + as opposed to parm setup. */ + rtx_note *b = emit_note (NOTE_INSN_FUNCTION_BEG); + + gcc_assert (NOTE_P (get_last_insn ())); + /* If function gets a static chain arg, store it. */ if (cfun->static_chain_decl) { @@ -5284,15 +5292,7 @@ expand_function_start (tree subr) update_nonlocal_goto_save_area (); } - /* The following was moved from init_function_start. - The move is supposed to make sdb output more accurate. */ - /* Indicate the beginning of the function body, - as opposed to parm setup. */ - emit_note (NOTE_INSN_FUNCTION_BEG); - - gcc_assert (NOTE_P (get_last_insn ())); - - parm_birth_insn = get_last_insn (); + parm_birth_insn = b; if (crtl->profile) { diff --git a/gcc/testsuite/gcc.dg/asan/pr81186.c b/gcc/testsuite/gcc.dg/asan/pr81186.c new file mode 100644 index 00000000000..7f0f672ca40 --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/pr81186.c @@ -0,0 +1,18 @@ +/* PR sanitizer/81186 */ +/* { dg-do run } */ + +int +main () +{ + __label__ l; + void f () + { + int a[123]; + + goto l; + } + + f (); +l: + return 0; +} -- 2.13.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-07-13 14:15 ` Martin Liška @ 2017-07-14 13:42 ` Michael Matz 2017-07-17 12:12 ` Martin Liška 0 siblings, 1 reply; 13+ messages in thread From: Michael Matz @ 2017-07-14 13:42 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 653 bytes --] Hi, On Thu, 13 Jul 2017, Martin Liška wrote: > Hopefully following patch will fix that. I returned to the first version > and saved/restored static_chain register before/after > __asan_stack_malloc. It should also work if you emit the parm_birth_note after the static chain is set up (not before it), but before you store into the nonlocal_goto_save_area. With that you don't need to worry about clobbering the incoming static chain with the asan setup. Can you test that? It would better reflect the intent of this note (the static chain being an implicit parameter, but the nonlocal_goto_save_area setup not being such). Ciao, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-07-14 13:42 ` Michael Matz @ 2017-07-17 12:12 ` Martin Liška 2017-07-17 13:15 ` Michael Matz 0 siblings, 1 reply; 13+ messages in thread From: Martin Liška @ 2017-07-17 12:12 UTC (permalink / raw) To: Michael Matz; +Cc: gcc-patches, Jakub Jelinek On 07/14/2017 03:42 PM, Michael Matz wrote: > Hi, > > On Thu, 13 Jul 2017, Martin Liška wrote: > >> Hopefully following patch will fix that. I returned to the first version >> and saved/restored static_chain register before/after >> __asan_stack_malloc. > > It should also work if you emit the parm_birth_note after the static chain > is set up (not before it), but before you store into the > nonlocal_goto_save_area. With that you don't need to worry about > clobbering the incoming static chain with the asan setup. Unfortunately it does not work. First asan_emit_stack_protection is executed, which creates: #0 0x00000000009850f4 in expand_used_vars () at ../../gcc/cfgexpand.c:2233 #1 0x0000000000992ab7 in (anonymous namespace)::pass_expand::execute (this=0x28c02f0, fun=0x2aaaac1b60b0) at ../../gcc/cfgexpand.c:6232 #2 0x0000000000e0d3a8 in execute_one_pass (pass=0x28c02f0) at ../../gcc/passes.c:2492 which does all the stack preparation (including the problematic call to __asan_stack_malloc_N). Note that this code still should be placed before parm_birth_note as we cant's say that params are ready before a fake stack is prepared. Then we generate code that loads the implicit chain argument: (gdb) p debug_rtx_list(get_insns(), 100) (note 1 0 37 NOTE_INSN_DELETED) (note 37 1 38 NOTE_INSN_FUNCTION_BEG) (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ]) (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 (nil)) (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) (const_int -584 [0xfffffffffffffdb8])) [0 S8 A64]) (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 (nil)) Which is problematic as using virtual-stack-vars which should point to fake stack done by AddressSanitizer in __asan_stack_malloc_N. That said both parts (ASAN fake stack init and CHAIN load from implicit argument) are before parm birth actions that should be aware each other. Thus my previous patch preserves the r10 register on x86_64. Thanks, Martin > > Can you test that? It would better reflect the intent of this note (the > static chain being an implicit parameter, but the nonlocal_goto_save_area > setup not being such). > > > Ciao, > Michael. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-07-17 12:12 ` Martin Liška @ 2017-07-17 13:15 ` Michael Matz 2017-07-18 8:38 ` Martin Liška 0 siblings, 1 reply; 13+ messages in thread From: Michael Matz @ 2017-07-17 13:15 UTC (permalink / raw) To: Martin Liška; +Cc: gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 2914 bytes --] Hello, On Mon, 17 Jul 2017, Martin Liška wrote: > which does all the stack preparation (including the problematic call to > __asan_stack_malloc_N). > > Note that this code still should be placed before parm_birth_note as we > cant's say that params are ready before a fake stack is prepared. Yes, understood. > Then we generate code that loads the implicit chain argument: > > (gdb) p debug_rtx_list(get_insns(), 100) > (note 1 0 37 NOTE_INSN_DELETED) > > (note 37 1 38 NOTE_INSN_FUNCTION_BEG) > > (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ]) > (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 > (nil)) > > (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) > (const_int -584 [0xfffffffffffffdb8])) [0 S8 A64]) > (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 > (nil)) > > Which is problematic as using virtual-stack-vars which should point to > fake stack done by AddressSanitizer in __asan_stack_malloc_N. If anything, then only the stack access is problematic, i.e. the last instruction. I don't understand why that should be problematic, though. Probably because I don't know much about the ASAN implementation. But why should there be something magic about using the non-asan stack? Most local variable accesses are rewritten to be in terms of the fake stack, but those that aren't could use the normal stack just fine, can't they? If that really is a problem then that could also be rectified by splitting the static_chain_decl in expand_function_start a bit, ala this: if (cfun->static_chain_decl) { all code except the last "if (!optimize) store-into-stack" } emit_note; parm_birth_insn = ... if (cfun->static_chain_decl && !optimize) { store into assign_stack_local } (requires moving some local variable to an outer scope, but hey). But what you say above mystifies me. You claim that access via virtual-stack-vars is problematic before the shadow stack is created by ASAN. But the whole parameter setup always uses such local stack storage whenever it needs. And those definitely happen before the ASAN setup. See the subroutines of assign_parms, (e.g. assign_parm_setup_block and assign_parm_setup_stack). You might need to use special function argument types or special ABIs to trigger this, though you should be able to find some cases to trigger also on i386 or x86_64. So, if the stack access for the static chain is problematic I don't see why the stack accesses for the parameters are not. And if they indeed are problematic, then something is confused within ASAN, and the fix for that confusion is not to move parm_birth_insn, but something else (I can't say what, as I don't know much about how ASAN is supposed to work in such situations). Ciao, Michael. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-07-17 13:15 ` Michael Matz @ 2017-07-18 8:38 ` Martin Liška 2017-07-25 10:06 ` Martin Liška 2017-07-25 12:49 ` Jakub Jelinek 0 siblings, 2 replies; 13+ messages in thread From: Martin Liška @ 2017-07-18 8:38 UTC (permalink / raw) To: Michael Matz; +Cc: gcc-patches, Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 4411 bytes --] On 07/17/2017 03:15 PM, Michael Matz wrote: > Hello, > > On Mon, 17 Jul 2017, Martin Liška wrote: > >> which does all the stack preparation (including the problematic call to >> __asan_stack_malloc_N). >> >> Note that this code still should be placed before parm_birth_note as we >> cant's say that params are ready before a fake stack is prepared. > > Yes, understood. > >> Then we generate code that loads the implicit chain argument: >> >> (gdb) p debug_rtx_list(get_insns(), 100) >> (note 1 0 37 NOTE_INSN_DELETED) >> >> (note 37 1 38 NOTE_INSN_FUNCTION_BEG) >> >> (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ]) >> (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 >> (nil)) >> >> (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) >> (const_int -584 [0xfffffffffffffdb8])) [0 S8 A64]) >> (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 >> (nil)) >> >> Which is problematic as using virtual-stack-vars which should point to >> fake stack done by AddressSanitizer in __asan_stack_malloc_N. > > If anything, then only the stack access is problematic, i.e. the last > instruction. I don't understand why that should be problematic, though. Hi. Thanks one more time, it's really educative this PR and whole problematic of function prologue. So short answer for your email: marking parm_birth_insn after static chain init solves the problem :) It's because: (insn 2 1 3 (set (reg/f:DI 100 [ CHAIN.2 ]) (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1 (nil)) (insn 3 2 4 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) (const_int -8 [0xfffffffffffffff8])) [0 S8 A64]) (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1 (nil)) is just storage of &FRAME.0 from caller where content of the FRAME struct lives on stack (and thus on shadow stack). That said it's perfectly fine to store &CHAIN to real stack of callee. Thus I'm going to test attached patch. P.S. One interesting side effect of how static chain is implemented: Consider: int main () { __label__ l; int buffer[100]; void f () { int a[123]; *(&buffer[0] - 4) = 123; goto l; } f (); l: return 0; } It's funny that *(&buffer[0] - 4) actually corrupts __nl_goto_buf and we end up with a dead signal: ASAN:DEADLYSIGNAL ================================================================= ==30888==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x000000000000 sp 0x7ffe0000049b T0) Thanks, Martin > Probably because I don't know much about the ASAN implementation. But why > should there be something magic about using the non-asan stack? Most > local variable accesses are rewritten to be in terms of the fake stack, > but those that aren't could use the normal stack just fine, can't they? > > If that really is a problem then that could also be rectified by splitting > the static_chain_decl in expand_function_start a bit, ala this: > > if (cfun->static_chain_decl) { > all code except the last "if (!optimize) store-into-stack" > } > emit_note; parm_birth_insn = ... > if (cfun->static_chain_decl && !optimize) { > store into assign_stack_local > } > > (requires moving some local variable to an outer scope, but hey). > > But what you say above mystifies me. You claim that access via > virtual-stack-vars is problematic before the shadow stack is created by > ASAN. But the whole parameter setup always uses such local stack storage > whenever it needs. And those definitely happen before the ASAN setup. > See the subroutines of assign_parms, (e.g. assign_parm_setup_block and > assign_parm_setup_stack). You might need to use special function argument > types or special ABIs to trigger this, though you should be able to find > some cases to trigger also on i386 or x86_64. > > So, if the stack access for the static chain is problematic I don't see > why the stack accesses for the parameters are not. And if they indeed are > problematic, then something is confused within ASAN, and the fix for that > confusion is not to move parm_birth_insn, but something else (I can't say > what, as I don't know much about how ASAN is supposed to work in such > situations). > > > Ciao, > Michael. > [-- Attachment #2: 0001-Move-static-chain-and-non-local-goto-init-after-NOTE.patch --] [-- Type: text/x-patch, Size: 2350 bytes --] From 13d08eb4c7d1ff7cddd130acad405ec343cb826f Mon Sep 17 00:00:00 2001 From: marxin <mliska@suse.cz> Date: Thu, 13 Jul 2017 13:37:47 +0200 Subject: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG gcc/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * function.c (expand_function_start): Set parm_birth_insn after static chain is initialized. gcc/testsuite/ChangeLog: 2017-06-27 Martin Liska <mliska@suse.cz> PR sanitize/81186 * gcc.dg/asan/pr81186.c: New test. --- gcc/function.c | 20 ++++++++++---------- gcc/testsuite/gcc.dg/asan/pr81186.c | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 10 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/asan/pr81186.c diff --git a/gcc/function.c b/gcc/function.c index f625489205b..9cfe58afe90 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -5263,6 +5263,16 @@ expand_function_start (tree subr) } } + /* The following was moved from init_function_start. + The move is supposed to make sdb output more accurate. */ + /* Indicate the beginning of the function body, + as opposed to parm setup. */ + emit_note (NOTE_INSN_FUNCTION_BEG); + + gcc_assert (NOTE_P (get_last_insn ())); + + parm_birth_insn = get_last_insn (); + /* If the function receives a non-local goto, then store the bits we need to restore the frame pointer. */ if (cfun->nonlocal_goto_save_area) @@ -5284,16 +5294,6 @@ expand_function_start (tree subr) update_nonlocal_goto_save_area (); } - /* The following was moved from init_function_start. - The move is supposed to make sdb output more accurate. */ - /* Indicate the beginning of the function body, - as opposed to parm setup. */ - emit_note (NOTE_INSN_FUNCTION_BEG); - - gcc_assert (NOTE_P (get_last_insn ())); - - parm_birth_insn = get_last_insn (); - if (crtl->profile) { #ifdef PROFILE_HOOK diff --git a/gcc/testsuite/gcc.dg/asan/pr81186.c b/gcc/testsuite/gcc.dg/asan/pr81186.c new file mode 100644 index 00000000000..7f0f672ca40 --- /dev/null +++ b/gcc/testsuite/gcc.dg/asan/pr81186.c @@ -0,0 +1,18 @@ +/* PR sanitizer/81186 */ +/* { dg-do run } */ + +int +main () +{ + __label__ l; + void f () + { + int a[123]; + + goto l; + } + + f (); +l: + return 0; +} -- 2.13.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-07-18 8:38 ` Martin Liška @ 2017-07-25 10:06 ` Martin Liška 2017-07-25 12:49 ` Jakub Jelinek 1 sibling, 0 replies; 13+ messages in thread From: Martin Liška @ 2017-07-25 10:06 UTC (permalink / raw) To: Michael Matz; +Cc: gcc-patches, Jakub Jelinek PING^1 Jakub can you please take a look? I would like to have it in 7.2 if possible. Thanks, Martin On 07/18/2017 10:38 AM, Martin Liška wrote: > On 07/17/2017 03:15 PM, Michael Matz wrote: >> Hello, >> >> On Mon, 17 Jul 2017, Martin Liška wrote: >> >>> which does all the stack preparation (including the problematic call to >>> __asan_stack_malloc_N). >>> >>> Note that this code still should be placed before parm_birth_note as we >>> cant's say that params are ready before a fake stack is prepared. >> >> Yes, understood. >> >>> Then we generate code that loads the implicit chain argument: >>> >>> (gdb) p debug_rtx_list(get_insns(), 100) >>> (note 1 0 37 NOTE_INSN_DELETED) >>> >>> (note 37 1 38 NOTE_INSN_FUNCTION_BEG) >>> >>> (insn 38 37 39 (set (reg/f:DI 94 [ CHAIN.1 ]) >>> (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 >>> (nil)) >>> >>> (insn 39 38 0 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) >>> (const_int -584 [0xfffffffffffffdb8])) [0 S8 A64]) >>> (reg:DI 39 r10 [ CHAIN.1 ])) "/home/marxin/Programming/gcc/gcc/testsuite/gcc.dg/asan/pr81186.c":9 -1 >>> (nil)) >>> >>> Which is problematic as using virtual-stack-vars which should point to >>> fake stack done by AddressSanitizer in __asan_stack_malloc_N. >> >> If anything, then only the stack access is problematic, i.e. the last >> instruction. I don't understand why that should be problematic, though. > > Hi. > > Thanks one more time, it's really educative this PR and whole problematic of function prologue. > So short answer for your email: marking parm_birth_insn after static chain init solves the problem :) > It's because: > > (insn 2 1 3 (set (reg/f:DI 100 [ CHAIN.2 ]) > (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1 > (nil)) > > (insn 3 2 4 (set (mem/c:DI (plus:DI (reg/f:DI 82 virtual-stack-vars) > (const_int -8 [0xfffffffffffffff8])) [0 S8 A64]) > (reg:DI 39 r10 [ CHAIN.2 ])) "/tmp/nested.c":6 -1 > (nil)) > > is just storage of &FRAME.0 from caller where content of the FRAME struct lives on stack (and thus on > shadow stack). That said it's perfectly fine to store &CHAIN to real stack of callee. > > Thus I'm going to test attached patch. > > P.S. One interesting side effect of how static chain is implemented: > > Consider: > > int > main () > { > __label__ l; > int buffer[100]; > void f () > { > int a[123]; > *(&buffer[0] - 4) = 123; > > goto l; > } > > f (); > l: > return 0; > } > > It's funny that *(&buffer[0] - 4) actually corrupts __nl_goto_buf and we end up with a > dead signal: > > ASAN:DEADLYSIGNAL > ================================================================= > ==30888==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x000000000000 bp 0x000000000000 sp 0x7ffe0000049b T0) > > Thanks, > Martin > >> Probably because I don't know much about the ASAN implementation. But why >> should there be something magic about using the non-asan stack? Most >> local variable accesses are rewritten to be in terms of the fake stack, >> but those that aren't could use the normal stack just fine, can't they? >> >> If that really is a problem then that could also be rectified by splitting >> the static_chain_decl in expand_function_start a bit, ala this: >> >> if (cfun->static_chain_decl) { >> all code except the last "if (!optimize) store-into-stack" >> } >> emit_note; parm_birth_insn = ... >> if (cfun->static_chain_decl && !optimize) { >> store into assign_stack_local >> } >> >> (requires moving some local variable to an outer scope, but hey). >> >> But what you say above mystifies me. You claim that access via >> virtual-stack-vars is problematic before the shadow stack is created by >> ASAN. But the whole parameter setup always uses such local stack storage >> whenever it needs. And those definitely happen before the ASAN setup. >> See the subroutines of assign_parms, (e.g. assign_parm_setup_block and >> assign_parm_setup_stack). You might need to use special function argument >> types or special ABIs to trigger this, though you should be able to find >> some cases to trigger also on i386 or x86_64. >> >> So, if the stack access for the static chain is problematic I don't see >> why the stack accesses for the parameters are not. And if they indeed are >> problematic, then something is confused within ASAN, and the fix for that >> confusion is not to move parm_birth_insn, but something else (I can't say >> what, as I don't know much about how ASAN is supposed to work in such >> situations). >> >> >> Ciao, >> Michael. >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-07-18 8:38 ` Martin Liška 2017-07-25 10:06 ` Martin Liška @ 2017-07-25 12:49 ` Jakub Jelinek 2017-07-27 9:29 ` Martin Liška 1 sibling, 1 reply; 13+ messages in thread From: Jakub Jelinek @ 2017-07-25 12:49 UTC (permalink / raw) To: Martin Liška; +Cc: Michael Matz, gcc-patches On Tue, Jul 18, 2017 at 10:38:50AM +0200, Martin LiÅ¡ka wrote: > 2017-06-27 Martin Liska <mliska@suse.cz> > > PR sanitize/81186 8 spaces instead of tab? > * function.c (expand_function_start): Set parm_birth_insn after > static chain is initialized. I don't like this description, after all, parm_birth_insn was set after static chain initialization before too (just not right after it in some cases). The important change is that you've moved parm_birth_insn before the nonlocal_goto_save_area setup code, so IMHO the ChangeLog entry should say that. As for the patch itself, there are many spots which insert some code before or after parm_birth_insn or spots tied to the NOTE_INSN_FUNCTION_BEG note, but I'd hope nothing inserted there can actually call functions that perform non-local gotos, so I think the patch is fine. And for debug info experience which is also related to NOTE_INSN_FUNCTION_BEG, I think the nl goto save area is nothing that can be seen in the debugger unless you know where it is, so the only change might be if you put a breakpoint on the end of prologue (i.e. NOTE_INSN_FUNCTION_BEG) and call from inferios some function that performs a non-local goto. I think there are no barriers on that initialization anyway, so scheduler can move it around. Thus, ok for trunk/7.2 with the above suggested ChangeLog change. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186). 2017-07-25 12:49 ` Jakub Jelinek @ 2017-07-27 9:29 ` Martin Liška 0 siblings, 0 replies; 13+ messages in thread From: Martin Liška @ 2017-07-27 9:29 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Michael Matz, gcc-patches On 07/25/2017 02:49 PM, Jakub Jelinek wrote: > On Tue, Jul 18, 2017 at 10:38:50AM +0200, Martin LiÅ¡ka wrote: >> 2017-06-27 Martin Liska <mliska@suse.cz> >> >> PR sanitize/81186 > > 8 spaces instead of tab? > >> * function.c (expand_function_start): Set parm_birth_insn after >> static chain is initialized. > > I don't like this description, after all, parm_birth_insn was set > after static chain initialization before too (just not right after it > in some cases). The important change is that you've moved parm_birth_insn > before the nonlocal_goto_save_area setup code, so IMHO the ChangeLog entry > should say that. Both notes fixed and the patch has been also installed to GCC 7 branch. Thanks, Martin > > As for the patch itself, there are many spots which insert some code > before or after parm_birth_insn or spots tied to the NOTE_INSN_FUNCTION_BEG > note, but I'd hope nothing inserted there can actually call functions that > perform non-local gotos, so I think the patch is fine. And for debug info > experience which is also related to NOTE_INSN_FUNCTION_BEG, I think the nl > goto save area is nothing that can be seen in the debugger unless you know > where it is, so the only change might be if you put a breakpoint on the end > of prologue (i.e. NOTE_INSN_FUNCTION_BEG) and call from inferios some > function that performs a non-local goto. I think there are no barriers > on that initialization anyway, so scheduler can move it around. > > Thus, ok for trunk/7.2 with the above suggested ChangeLog change. > > Jakub > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-07-27 9:29 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-27 13:05 [PATCH] Move static chain and non-local goto init after NOTE_INSN_FUNCTION_BEG (PR sanitize/81186) Martin Liška 2017-06-27 15:29 ` Michael Matz 2017-06-28 11:11 ` Martin Liška 2017-06-30 14:03 ` Michael Matz 2017-06-30 14:33 ` Martin Liška 2017-07-13 14:15 ` Martin Liška 2017-07-14 13:42 ` Michael Matz 2017-07-17 12:12 ` Martin Liška 2017-07-17 13:15 ` Michael Matz 2017-07-18 8:38 ` Martin Liška 2017-07-25 10:06 ` Martin Liška 2017-07-25 12:49 ` Jakub Jelinek 2017-07-27 9:29 ` Martin Liška
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).