From: Alexander Popov <alex.popov@linux.com> To: Will Deacon <will.deacon@arm.com>, Laura Abbott <labbott@redhat.com>, Kees Cook <keescook@chromium.org>, Mark Rutland <mark.rutland@arm.com>, Ard Biesheuvel <ard.biesheuvel@linaro.org>, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, kernel-hardening@lists.openwall.com, richard.sandiford@arm.com Subject: Re: [PATCH 1/2] stackleak: Update for arm64 Date: Wed, 28 Feb 2018 18:09:54 +0300 [thread overview] Message-ID: <5d10c6ad-b4b6-eb27-79f0-ca235b73e7eb@linux.com> (raw) In-Reply-To: <874lm2d96q.fsf@e105548-lin.cambridge.arm.com> On 27.02.2018 13:21, Richard Sandiford wrote: > Hi Alexander, > > Sorry for the slow reply, been caught up in an office move. Thank you very much for the review, Richard! > Alexander Popov <alex.popov@linux.com> writes: >> Would you be so kind to take a look at the whole STACKLEAK plugin? >> http://www.openwall.com/lists/kernel-hardening/2018/02/16/4 >> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/gcc-plugin/stackleak&id=57a0a6763b12e82dd462593d0f42be610e93cdc9 >> >> It's not very big. I documented it in detail. I would be really grateful for the >> review! > > Looks good to me FWIW. Just a couple of minor things: > >> + /* >> + * 1. Loop through the GIMPLE statements in each of cfun basic blocks. >> + * cfun is a global variable which represents the function that is >> + * currently processed. >> + */ >> + FOR_EACH_BB_FN(bb, cfun) { >> + for (gsi = gsi_start_bb(bb); !gsi_end_p(gsi); gsi_next(&gsi)) { >> + gimple stmt; >> + >> + stmt = gsi_stmt(gsi); >> + >> + /* Leaf function is a function which makes no calls */ >> + if (is_gimple_call(stmt)) >> + is_leaf = false; > > It's probably not going to matter in practice, but it might be worth > emphasising in the comments that this leafness is only approximate. That's important, thank you! May I ask why you think it's not going to matter in practice? > It will sometimes be a false positive, because we could still > end up creating calls to libgcc functions from non-call statements > (or for target-specific reasons). It can also be a false negative, > since call statements can be to built-in or internal functions that > end up being open-coded. Oh, that raises the question: how does this leafness inaccuracy affect in my particular case? is_leaf is currently used for finding the special cases to skip the track_stack() call insertion: /* * Special cases to skip the instrumentation. * * Taking the address of static inline functions materializes them, * but we mustn't instrument some of them as the resulting stack * alignment required by the function call ABI will break other * assumptions regarding the expected (but not otherwise enforced) * register clobbering ABI. * * Case in point: native_save_fl on amd64 when optimized for size * clobbers rdx if it were instrumented here. * * TODO: any more special cases? */ if (is_leaf && !TREE_PUBLIC(current_function_decl) && DECL_DECLARED_INLINE_P(current_function_decl)) { return 0; } And now it seems to me that the stackleak plugin should not instrument all static inline functions, regardless of is_leaf. Do you agree? >> + /* >> + * The stackleak_final pass should be executed before the "final" pass, >> + * which turns the RTL (Register Transfer Language) into assembly. >> + */ >> + PASS_INFO(stackleak_final, "final", 1, PASS_POS_INSERT_BEFORE); > > This might be too late, since it happens e.g. after addresses have > been calculated for branch ranges, and after machine-specific passes > (e.g. bundling on ia64). > > The stack size is final after reload, so inserting the pass after that > might be better. Thanks for that notice. May I ask for the additional clarification? I specified -fdump-passes and see a lot of passes between reload and final: ... rtl-sched1 : OFF rtl-ira : ON rtl-reload : ON rtl-vzeroupper : OFF *all-postreload : OFF rtl-postreload : OFF rtl-gcse2 : OFF rtl-split2 : ON rtl-ree : ON rtl-cmpelim : OFF rtl-btl1 : OFF rtl-pro_and_epilogue : ON rtl-dse2 : ON rtl-csa : ON rtl-jump2 : ON rtl-compgotos : ON rtl-sched_fusion : OFF rtl-peephole2 : ON rtl-ce3 : ON rtl-rnreg : OFF rtl-cprop_hardreg : ON rtl-rtl_dce : ON rtl-bbro : ON rtl-btl2 : OFF *leaf_regs : ON rtl-split4 : ON rtl-sched2 : ON *stack_regs : ON rtl-split3 : OFF rtl-stack : ON *all-late_compilation : OFF rtl-alignments : ON rtl-vartrack : ON *free_cfg : ON rtl-mach : ON rtl-barriers : ON rtl-dbr : OFF rtl-split5 : OFF rtl-eh_ranges : OFF rtl-shorten : ON rtl-nothrow : ON rtl-dwarf2 : ON rtl-stackleak_final : ON rtl-final : ON rtl-dfinish : ON clean_state : ON Where exactly would you recommend me to insert the stackleak_final pass, which removes the unneeded track_stack() calls? Best regards, Alexander
WARNING: multiple messages have this Message-ID (diff)
From: alex.popov@linux.com (Alexander Popov) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/2] stackleak: Update for arm64 Date: Wed, 28 Feb 2018 18:09:54 +0300 [thread overview] Message-ID: <5d10c6ad-b4b6-eb27-79f0-ca235b73e7eb@linux.com> (raw) In-Reply-To: <874lm2d96q.fsf@e105548-lin.cambridge.arm.com> On 27.02.2018 13:21, Richard Sandiford wrote: > Hi Alexander, > > Sorry for the slow reply, been caught up in an office move. Thank you very much for the review, Richard! > Alexander Popov <alex.popov@linux.com> writes: >> Would you be so kind to take a look at the whole STACKLEAK plugin? >> http://www.openwall.com/lists/kernel-hardening/2018/02/16/4 >> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=kspp/gcc-plugin/stackleak&id=57a0a6763b12e82dd462593d0f42be610e93cdc9 >> >> It's not very big. I documented it in detail. I would be really grateful for the >> review! > > Looks good to me FWIW. Just a couple of minor things: > >> + /* >> + * 1. Loop through the GIMPLE statements in each of cfun basic blocks. >> + * cfun is a global variable which represents the function that is >> + * currently processed. >> + */ >> + FOR_EACH_BB_FN(bb, cfun) { >> + for (gsi = gsi_start_bb(bb); !gsi_end_p(gsi); gsi_next(&gsi)) { >> + gimple stmt; >> + >> + stmt = gsi_stmt(gsi); >> + >> + /* Leaf function is a function which makes no calls */ >> + if (is_gimple_call(stmt)) >> + is_leaf = false; > > It's probably not going to matter in practice, but it might be worth > emphasising in the comments that this leafness is only approximate. That's important, thank you! May I ask why you think it's not going to matter in practice? > It will sometimes be a false positive, because we could still > end up creating calls to libgcc functions from non-call statements > (or for target-specific reasons). It can also be a false negative, > since call statements can be to built-in or internal functions that > end up being open-coded. Oh, that raises the question: how does this leafness inaccuracy affect in my particular case? is_leaf is currently used for finding the special cases to skip the track_stack() call insertion: /* * Special cases to skip the instrumentation. * * Taking the address of static inline functions materializes them, * but we mustn't instrument some of them as the resulting stack * alignment required by the function call ABI will break other * assumptions regarding the expected (but not otherwise enforced) * register clobbering ABI. * * Case in point: native_save_fl on amd64 when optimized for size * clobbers rdx if it were instrumented here. * * TODO: any more special cases? */ if (is_leaf && !TREE_PUBLIC(current_function_decl) && DECL_DECLARED_INLINE_P(current_function_decl)) { return 0; } And now it seems to me that the stackleak plugin should not instrument all static inline functions, regardless of is_leaf. Do you agree? >> + /* >> + * The stackleak_final pass should be executed before the "final" pass, >> + * which turns the RTL (Register Transfer Language) into assembly. >> + */ >> + PASS_INFO(stackleak_final, "final", 1, PASS_POS_INSERT_BEFORE); > > This might be too late, since it happens e.g. after addresses have > been calculated for branch ranges, and after machine-specific passes > (e.g. bundling on ia64). > > The stack size is final after reload, so inserting the pass after that > might be better. Thanks for that notice. May I ask for the additional clarification? I specified -fdump-passes and see a lot of passes between reload and final: ... rtl-sched1 : OFF rtl-ira : ON rtl-reload : ON rtl-vzeroupper : OFF *all-postreload : OFF rtl-postreload : OFF rtl-gcse2 : OFF rtl-split2 : ON rtl-ree : ON rtl-cmpelim : OFF rtl-btl1 : OFF rtl-pro_and_epilogue : ON rtl-dse2 : ON rtl-csa : ON rtl-jump2 : ON rtl-compgotos : ON rtl-sched_fusion : OFF rtl-peephole2 : ON rtl-ce3 : ON rtl-rnreg : OFF rtl-cprop_hardreg : ON rtl-rtl_dce : ON rtl-bbro : ON rtl-btl2 : OFF *leaf_regs : ON rtl-split4 : ON rtl-sched2 : ON *stack_regs : ON rtl-split3 : OFF rtl-stack : ON *all-late_compilation : OFF rtl-alignments : ON rtl-vartrack : ON *free_cfg : ON rtl-mach : ON rtl-barriers : ON rtl-dbr : OFF rtl-split5 : OFF rtl-eh_ranges : OFF rtl-shorten : ON rtl-nothrow : ON rtl-dwarf2 : ON rtl-stackleak_final : ON rtl-final : ON rtl-dfinish : ON clean_state : ON Where exactly would you recommend me to insert the stackleak_final pass, which removes the unneeded track_stack() calls? Best regards, Alexander
next prev parent reply other threads:[~2018-02-28 15:09 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-02-16 18:10 [PATCH RFC v8 0/6] Introduce the STACKLEAK feature and a test for it Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 1/6] x86/entry: Add STACKLEAK erasing the kernel stack at the end of syscalls Alexander Popov 2018-02-21 13:24 ` Borislav Petkov 2018-02-21 21:49 ` Alexander Popov 2018-02-22 19:14 ` Borislav Petkov 2018-02-22 20:24 ` Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 2/6] gcc-plugins: Add STACKLEAK plugin for tracking the kernel stack Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 3/6] x86/entry: Erase kernel stack in syscall_trace_enter() Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 4/6] lkdtm: Add a test for STACKLEAK Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 5/6] fs/proc: Show STACKLEAK metrics in the /proc file system Alexander Popov 2018-02-16 18:10 ` [PATCH RFC v8 6/6] doc: self-protection: Add information about STACKLEAK feature Alexander Popov 2018-02-20 10:29 ` [PATCH RFC v8 0/6] Introduce the STACKLEAK feature and a test for it Alexander Popov 2018-02-20 23:17 ` Kees Cook 2018-02-20 23:33 ` Laura Abbott 2018-02-21 1:13 ` [PATCH 0/2] Stackleak for arm64 Laura Abbott 2018-02-21 1:13 ` Laura Abbott 2018-02-21 1:13 ` [PATCH 1/2] stackleak: Update " Laura Abbott 2018-02-21 1:13 ` Laura Abbott 2018-02-22 16:58 ` Will Deacon 2018-02-22 16:58 ` Will Deacon 2018-02-22 19:22 ` Alexander Popov 2018-02-22 19:22 ` Alexander Popov 2018-02-27 10:21 ` Richard Sandiford 2018-02-27 10:21 ` Richard Sandiford 2018-02-27 10:21 ` Richard Sandiford 2018-02-28 15:09 ` Alexander Popov [this message] 2018-02-28 15:09 ` Alexander Popov 2018-03-01 10:33 ` Richard Sandiford 2018-03-01 10:33 ` Richard Sandiford 2018-03-01 10:33 ` Richard Sandiford 2018-03-02 11:14 ` Alexander Popov 2018-03-02 11:14 ` Alexander Popov 2018-02-22 19:38 ` Laura Abbott 2018-02-22 19:38 ` Laura Abbott 2018-02-21 1:13 ` [PATCH 2/2] arm64: Clear the stack Laura Abbott 2018-02-21 1:13 ` Laura Abbott 2018-02-21 15:38 ` Mark Rutland 2018-02-21 15:38 ` Mark Rutland 2018-02-21 23:53 ` Laura Abbott 2018-02-21 23:53 ` Laura Abbott 2018-02-22 1:35 ` Laura Abbott 2018-02-22 1:35 ` Laura Abbott 2018-02-21 14:48 ` [PATCH 0/2] Stackleak for arm64 Alexander Popov 2018-02-21 14:48 ` Alexander Popov 2018-02-21 10:05 ` [PATCH RFC v8 0/6] Introduce the STACKLEAK feature and a test for it Borislav Petkov 2018-02-21 15:09 ` Alexander Popov 2018-02-21 14:43 ` Alexander Popov 2018-02-22 1:43 ` Laura Abbott 2018-02-22 23:14 ` [PATCH 0/2] Update stackleak for gcc-8 Laura Abbott 2018-02-22 23:14 ` [PATCH 1/2] gcc-plugins: Update cgraph_create_edge " Laura Abbott 2018-02-22 23:40 ` Kees Cook 2018-02-23 17:30 ` Laura Abbott 2018-02-24 12:36 ` Alexander Popov 2018-02-22 23:14 ` [PATCH 2/2] gcc-plugins: stackleak: Update " Laura Abbott 2018-02-24 14:04 ` Alexander Popov 2018-02-26 21:51 ` Laura Abbott 2018-02-27 10:30 ` Richard Sandiford 2018-02-28 10:27 ` Alexander Popov 2018-02-22 23:43 ` [PATCH 0/2] Update stackleak " Kees Cook 2018-05-02 20:33 [PATCH 0/2] Stackleak for arm64 Laura Abbott 2018-05-02 20:33 ` [PATCH 1/2] stackleak: Update " Laura Abbott 2018-05-02 20:33 ` Laura Abbott
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5d10c6ad-b4b6-eb27-79f0-ca235b73e7eb@linux.com \ --to=alex.popov@linux.com \ --cc=ard.biesheuvel@linaro.org \ --cc=keescook@chromium.org \ --cc=kernel-hardening@lists.openwall.com \ --cc=labbott@redhat.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=richard.sandiford@arm.com \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.