From: Maxim Ostapenko <m.ostapenko@samsung.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Richard Biener <rguenther@suse.de>,
Yuri Gribov <tetra2005@gmail.com>
Subject: Re: [PATCH v2][ASAN] Implement dynamic allocas/VLAs sanitization.
Date: Fri, 30 Jun 2017 16:37:00 -0000 [thread overview]
Message-ID: <59567E34.1010001@samsung.com> (raw)
In-Reply-To: <20170629123527.GC2123@tucnak>
Hi,
On 29/06/17 15:35, Jakub Jelinek wrote:
> Hi!
>
> Sorry for the review delay.
>
> On Mon, Jun 26, 2017 at 03:49:23PM +0300, Maxim Ostapenko wrote:
>> (handle_builtin_stackrestore): Likewise.
> The function is called with _ between stack and restore.
>
>> * match.pd: Add new pattern.
> Unless the patch relies on this, I think it should be posted separately
> and reviewed by Richard.
OK, good point, will remove from this patch.
>
>> @@ -245,6 +246,7 @@ along with GCC; see the file COPYING3. If not see
>> static unsigned HOST_WIDE_INT asan_shadow_offset_value;
>> static bool asan_shadow_offset_computed;
>> static vec<char *> sanitized_sections;
>> +static tree last_alloca_addr = NULL_TREE;
> You are shadowing this variable in multiple places. Either rename it to
> something different, or rename the results of get_last_alloca_addr.
> And the " = NULL_TREE" part is not needed.
Err, thanks, will fix it.
>
>>
>> /* Set of variable declarations that are going to be guarded by
>> use-after-scope sanitizer. */
>> @@ -529,11 +531,183 @@ get_mem_ref_of_assignment (const gassign *assignment,
>> return true;
>> }
>>
>> +/* Return address of last allocated dynamic alloca. */
>> +
>> +static tree
>> +get_last_alloca_addr ()
>> +{
>> + if (last_alloca_addr)
>> + return last_alloca_addr;
>> +
>> + gimple_seq seq = NULL;
>> + gassign *g;
>> +
>> + last_alloca_addr = create_tmp_reg (ptr_type_node, "last_alloca_addr");
>> + g = gimple_build_assign (last_alloca_addr, NOP_EXPR,
>> + build_int_cst (ptr_type_node, 0));
> Instead of build_int_cst (ptr_type_node, 0) you should use
> null_pointer_node. And the NOP_EXPR there is just wrong, either it
> should be gimple_build_assign (last_alloca_addr, null_pointer_node);
> or gimple_build_assign (last_alloca_addr, INTEGER_CST, null_pointer_node);
>
>> + gimple_seq_add_stmt_without_update (&seq, g);
> Why the seq stuff at all? You have a single stmt you want to insert on
> edge.
Right, will fix it.
>> +
>> + edge e = single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun));
>> + gsi_insert_seq_on_edge_immediate (e, seq);
> So just use here
> gsi_insert_on_edge_immediate (e, g);
> instead.
>
>> + return last_alloca_addr;
>> +}
>> +
>> +/* Insert __asan_allocas_unpoison (top, bottom) call after
>> + __builtin_stack_restore (new_sp) call.
>> + The pseudocode of this routine should look like this:
>> + __builtin_stack_restore (new_sp);
>> + top = last_alloca_addr;
>> + bot = virtual_dynamic_stack_rtx;
>> + __asan_allocas_unpoison (top, bottom);
>> + last_alloca_addr = new_sp;
> The comment doesn't seem to agree with what you actually implement.
> There is no virtual_dynamic_stack_rtx during the asan pass, it is there
> only during expansion until the virtual regs are instantiated in the next
> pass. Furthermore, you have bot variable, but then use bottom.
Right, 'bottom' should be replaced by 'bot'.
Regarding to virtual_dynamic_stactk_rtx - as you correctly noted below,
second parameter of __asan_allocas_unpoison will be completely rewritten
in expand_builtin_alloca function by virtual_dynamic_stactk_rtx. Here
I've just passed new_sp as a dummy argument. This looks hacky, but the
problem is that for several architectures (e.g. PPC) we cannot use
new_sp as a 'bot' parameter because new_sp != last_alloca_addr on these
targets. Originally, I tried to do something like this:
top = last_alloca_addr;
bot = new_sp + STACK_DYNAMIC_OFFSET;
__asan_allocas_unpoison(top, bot);
last_alloca_addr = bot;
but I was confused by the fact that STACK_DYNAMIC_OFFSET becomes
available only after expansion to RTL. Rewriting 'bot' with
virtual_dynamic_stactk_rtx in RTL looks like the most feasible way how
to overcome this issue for me.
>
>> + tree last_alloca_addr = get_last_alloca_addr ();
> Here is the shadowing I talked about.
>
>> + tree restored_stack = gimple_call_arg (call, 0);
>> + tree fn = builtin_decl_implicit (BUILT_IN_ASAN_ALLOCAS_UNPOISON);
>> + gimple *g = gimple_build_call (fn, 2, last_alloca_addr, restored_stack);
> Here you clearly use the first argument of __builtin_stack_restore, which
> is that new_sp.
>
>> + gimple_seq_add_stmt_without_update (&seq, g);
> Why the messing up with sequences? Just insert the stmt immediately in,
> and the others as well.
>
>> + g = gimple_build_assign (last_alloca_addr, NOP_EXPR, restored_stack);
> This is again wrong, here you really don't know what restored_stack is,
> it could be SSA_NAME, but also something different, so you should use
> gimple_build_assign (last_alloca_addr, restored_stack);
> and let it figure out the rhs code.
Thanks, will fix.
>
>> + /* Extract lower bits from old_size. */
>> + wide_int size_nonzero_bits = get_nonzero_bits (old_size);
>> + wide_int rz_mask
>> + = wi::uhwi (redzone_mask, wi::get_precision (size_nonzero_bits));
>> + wide_int old_size_lower_bits = wi::bit_and (size_nonzero_bits, rz_mask);
>> +
>> + /* If alloca size is aligned to ASAN_RED_ZONE_SIZE, we don't need partial
>> + redzone. Otherwise, compute its size here. */
>> + if (wi::ne_p (old_size_lower_bits, 0))
>> + {
>> + /* misalign = size & (ASAN_RED_ZONE_SIZE - 1)
>> + partial_size = ASAN_RED_ZONE_SIZE - misalign. */
>> + g = gimple_build_assign (make_ssa_name (size_type_node, NULL),
>> + BIT_AND_EXPR, old_size, alloca_rz_mask);
>> + gimple_seq_add_stmt_without_update (&seq, g);
>> + tree misalign = gimple_assign_lhs (g);
>> + g = gimple_build_assign (make_ssa_name (size_type_node, NULL), MINUS_EXPR,
>> + redzone_size, misalign);
>> + gimple_seq_add_stmt_without_update (&seq, g);
> Again, why add the stmts into a seq first instead of just adding it
> immediately into the IL?
>> @@ -4962,6 +4962,20 @@ expand_builtin_alloca (tree exp)
>> return result;
>> }
>>
> Missing function comment here.
>
>> +static rtx
>> +expand_asan_emit_allocas_unpoison (tree exp)
>> +{
>> + tree arg0 = CALL_EXPR_ARG (exp, 0);
>> + rtx top = expand_expr (arg0, NULL_RTX, GET_MODE (virtual_stack_dynamic_rtx),
>> + EXPAND_NORMAL);
>> + rtx ret = init_one_libfunc ("__asan_allocas_unpoison");
>> + ret = emit_library_call_value (ret, NULL_RTX, LCT_NORMAL, ptr_mode, 2, top,
>> + TYPE_MODE (pointer_sized_int_node),
>> + virtual_stack_dynamic_rtx,
>> + TYPE_MODE (pointer_sized_int_node));
> I see you are here pretty much ignoring the old second argument and instead
> using a different one. But then the above mentioned comment should explain
> that, how you transform it during the asan pass and how you later change it
> during expansion.
Yeah, I'll add a comment about motivation.
>
>> + case BUILT_IN_ASAN_ALLOCAS_UNPOISON:
>> + target = expand_asan_emit_allocas_unpoison (exp);
>> + if (target)
>> + return target;
> Do you need this test, when it always returns non-NULL?
> Just return expand_asan_emit_allocas_unpoison (exp);
>
> Jakub
>
>
>
-Maxim
prev parent reply other threads:[~2017-06-30 16:37 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20170626124925eucas1p18c56742a07db5bb2dabbedd0e894aa0e@eucas1p1.samsung.com>
2017-06-26 12:49 ` Maxim Ostapenko
2017-06-29 12:35 ` Jakub Jelinek
2017-06-30 16:37 ` Maxim Ostapenko [this message]
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=59567E34.1010001@samsung.com \
--to=m.ostapenko@samsung.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=rguenther@suse.de \
--cc=tetra2005@gmail.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 read-only IMAP folder(s) and NNTP newsgroup(s).