lkml.org 
[lkml]   [2018]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 1/2] stackleak: Update for arm64
Date
Hi Alexander,

Sorry for the slow reply, been caught up in an office move.

Alexander Popov <alex.popov@linux.com> writes:
> Hello Will, Richard and GCC folks!
>
> On 22.02.2018 19:58, Will Deacon wrote:
>> On Tue, Feb 20, 2018 at 05:13:02PM -0800, Laura Abbott wrote:
>>>
>>> arm64 has another layer of indirection in the RTL.
>>> Account for this in the plugin.
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>> scripts/gcc-plugins/stackleak_plugin.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/scripts/gcc-plugins/stackleak_plugin.c b/scripts/gcc-plugins/stackleak_plugin.c
>>> index 6fc991c98d8b..7dfaa027423f 100644
>>> --- a/scripts/gcc-plugins/stackleak_plugin.c
>>> +++ b/scripts/gcc-plugins/stackleak_plugin.c
>>> @@ -244,6 +244,11 @@ static unsigned int stackleak_final_execute(void)
>>> * that insn.
>>> */
>>> body = PATTERN(insn);
>>> + /* arm64 is different */
>>> + if (GET_CODE(body) == PARALLEL) {
>>> + body = XEXP(body, 0);
>>> + body = XEXP(body, 0);
>>> + }
>>
>> Like most kernel developers, I don't know the first thing about GCC internals
>> so I asked our GCC team and Richard (CC'd) reckons this should be:
>>
>> if (GET_CODE(body) == PARALLEL)
>> body = XVECEXP(body, 0, 0);
>>
>> instead of the hunk above. Can you give that a go instead, please?
>
> Thanks a lot!
>
> 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.
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.

> + /*
> + * 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,
Richard

\
 
 \ /
  Last update: 2018-02-27 11:22    [W:0.095 / U:3.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site