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
next prev parent reply other threads:[~2018-02-28 15:09 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <dc76a745-3fa7-4023-dcc1-3df18c9461a6@redhat.com>
2018-02-21 1:13 ` [PATCH 0/2] Stackleak for arm64 Laura Abbott
2018-02-21 1:13 ` [PATCH 1/2] stackleak: Update " Laura Abbott
2018-02-22 16:58 ` Will Deacon
2018-02-22 19:22 ` Alexander Popov
2018-02-27 10:21 ` Richard Sandiford
2018-02-28 15:09 ` Alexander Popov [this message]
2018-03-01 10:33 ` Richard Sandiford
2018-03-02 11:14 ` Alexander Popov
2018-02-22 19:38 ` Laura Abbott
2018-02-21 1:13 ` [PATCH 2/2] arm64: Clear the stack Laura Abbott
2018-02-21 15:38 ` Mark Rutland
2018-02-21 23:53 ` Laura Abbott
2018-02-22 1:35 ` Laura Abbott
2018-02-21 14:48 ` [PATCH 0/2] Stackleak for arm64 Alexander Popov
2018-05-02 20:33 Laura Abbott
2018-05-02 20:33 ` [PATCH 1/2] stackleak: Update " 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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 NNTP newsgroup(s).