From patchwork Thu Apr 12 22:16:06 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 897813 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-476315-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="G1tf8rfD"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 40MbMw66v4z9s0R for ; Fri, 13 Apr 2018 08:35:38 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:mime-version :content-type; q=dns; s=default; b=TkSRn2ad7b2v3fo5WgFcdOvUUjJWO 3a6SLHslDhFIHpsKQ+ZWdksIu83ZNmJy31uZRSc/ej/3zw8M7UbPCp9mM6wK8LxR pMsFzAyDUKddFkGkmJaqOcN6wTnqH7eWEeMhZDJCQmnpGRKYGlpcoayLI9YMuf6r TUq7MrXL5W/vNo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:mime-version :content-type; s=default; bh=Vb/prTELjO/OVYfmqyG0DzDxnfM=; b=G1t f8rfDysggFrqrea7OTw8+oVAgr+6nTT+cQBNZcHobYkKoOhhPU7uN+lmoOZmjq2F 8yTUMMV47mAlodDzN2ugKElSrQiqaMnmfDTcO/t9YtRf2Iw1cQNirfgejPfsXeM4 pAo/bu9SSGPx97/ukQgtUjT12y2wLs1BsHHN7SMA= Received: (qmail 45330 invoked by alias); 12 Apr 2018 22:35:30 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 45304 invoked by uid 89); 12 Apr 2018 22:35:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-11.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.2 spammy=suffer, HTo:U*dodji X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 12 Apr 2018 22:35:27 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B55514022909 for ; Thu, 12 Apr 2018 22:35:21 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.36.118.110]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 64A9F2017F21 for ; Thu, 12 Apr 2018 22:35:20 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.15.2/8.15.2) with ESMTP id w3CMG8lA026771; Fri, 13 Apr 2018 00:16:08 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.15.2/8.15.2/Submit) id w3CMG6Fk026770; Fri, 13 Apr 2018 00:16:06 +0200 Date: Fri, 13 Apr 2018 00:16:06 +0200 From: Jakub Jelinek To: Jeff Law , Dodji Seketeli , Maxim Ostapenko Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] Fix -fsanitize=address VLA instrumentation (PR sanitizer/85230) Message-ID: <20180412221606.GU8577@tucnak> Reply-To: Jakub Jelinek MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.9.2 (2017-12-15) X-IsSubscribed: yes Hi! As mentioned in the PR, we need to unpoison the red zones when leaving a scope with VLA variable(s); this is done through __asan_allocas_unpoison call, unfortunately it is called after the __builtin_stack_restore which restores the stack pointer; now, if an interrupt comes in between the stack restore and the __asan_allocas_unpoison call, the interrupt handler might have some stack bytes marked as red zones in the shadow memory and might diagnose sanitizing error even when there is none in the original program. The following patch ought to fix this by swapping the two calls, so we first unpoison and only after it is unpoisoned in shadow memory release the stack. The second argument to the __asan_allocas_unpoison call is meant to be virtual_dynamic_stack_rtx after the __builtin_stack_restore, i.e. the new stack_pointer_rtx value + STACK_DYNAMIC_OFFSET (current_function_decl). As the STACK_DYNAMIC_OFFSET value isn't known until the vregs pass, the code used a hack where it ignored the second argument and replaced it by virtual_dynamic_stack_rtx. With the asan.c change below this doesn't work anymore, because virtual_dynamic_stack_rtx aka stack_pointer_rtx + STACK_DYNAMIC_OFFSET (current_function_decl) before the __builtin_stack_restore is a different value. The patch instead uses the argument passed to the __asan_allocas_unpoison at GIMPLE time, which is the same as passed to __builtin_stack_restore; this is the new stack_pointer_rtx value after __builtin_stack_restore. And, because we don't want that value, but that + STACK_DYNAMIC_OFFSET (current_function_decl), we compute arg1 + (virtual_dynamic_stack_rtx - stack_pointer_rtx) and let CSE/combiner optimize it into arg1 (on targets like x86_64 where STACK_DYNAMIC_OFFSET can be even 0 when not accumulating outgoing args or when that size is 0) or arg1 + some_constant. Bootstrapped on {x86_64,i686,powerpc64,powerpc64le,aarch64,s390x,armv7hl}-linux, regtested on {x86_64,i686,powerpc64,powerpc64le}-linux so far, but on the power* ones on virtual address space size that isn't really supported (likely https://github.com/google/sanitizers/issues/933#issuecomment-380058705 issue, so while nothing regresses there, pretty much all asan tests fail there before and after the patch); also tested successfully with asan.exp=alloca* on gcc110 and gcc112 on compile farm where it doesn't suffer from the VA issue. Ok if testing passes also on aarch64, s390x and armv7hl? 2018-04-12 Jakub Jelinek PR sanitizer/85230 * asan.c (handle_builtin_stack_restore): Adjust comment. Emit __asan_allocas_unpoison call and last_alloca_addr = new_sp before __builtin_stack_restore rather than after it. * builtins.c (expand_asan_emit_allocas_unpoison): Pass arg1 + (virtual_dynamic_stack_rtx - stack_pointer_rtx) as second argument instead of virtual_dynamic_stack_rtx. Jakub --- gcc/asan.c.jj 2018-01-09 21:53:38.821577722 +0100 +++ gcc/asan.c 2018-04-12 13:22:59.166095523 +0200 @@ -554,14 +554,14 @@ get_last_alloca_addr () return last_alloca_addr; } -/* Insert __asan_allocas_unpoison (top, bottom) call after +/* Insert __asan_allocas_unpoison (top, bottom) call before __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 = new_sp; __asan_allocas_unpoison (top, bot); last_alloca_addr = new_sp; + __builtin_stack_restore (new_sp); In general, we can't use new_sp as bot parameter because on some architectures SP has non zero offset from dynamic stack area. Moreover, on some architectures this offset (STACK_DYNAMIC_OFFSET) becomes known for each @@ -570,9 +570,8 @@ get_last_alloca_addr () http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#DYNAM-STACK. To overcome the issue we use following trick: pass new_sp as a second parameter to __asan_allocas_unpoison and rewrite it during expansion with - virtual_dynamic_stack_rtx later in expand_asan_emit_allocas_unpoison - function. -*/ + new_sp + (virtual_dynamic_stack_rtx - sp) later in + expand_asan_emit_allocas_unpoison function. */ static void handle_builtin_stack_restore (gcall *call, gimple_stmt_iterator *iter) @@ -584,9 +583,9 @@ handle_builtin_stack_restore (gcall *cal 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, restored_stack); - gsi_insert_after (iter, g, GSI_NEW_STMT); + gsi_insert_before (iter, g, GSI_SAME_STMT); g = gimple_build_assign (last_alloca, restored_stack); - gsi_insert_after (iter, g, GSI_NEW_STMT); + gsi_insert_before (iter, g, GSI_SAME_STMT); } /* Deploy and poison redzones around __builtin_alloca call. To do this, we --- gcc/builtins.c.jj 2018-04-04 21:33:20.530639395 +0200 +++ gcc/builtins.c 2018-04-12 13:35:34.328395156 +0200 @@ -5068,18 +5068,24 @@ expand_builtin_alloca (tree exp) return result; } -/* Emit a call to __asan_allocas_unpoison call in EXP. Replace second argument - of the call with virtual_stack_dynamic_rtx because in asan pass we emit a - dummy value into second parameter relying on this function to perform the - change. See motivation for this in comment to handle_builtin_stack_restore - function. */ +/* Emit a call to __asan_allocas_unpoison call in EXP. Add to second argument + of the call virtual_stack_dynamic_rtx - stack_pointer_rtx, which is the + STACK_DYNAMIC_OFFSET value. See motivation for this in comment to + handle_builtin_stack_restore function. */ static rtx expand_asan_emit_allocas_unpoison (tree exp) { tree arg0 = CALL_EXPR_ARG (exp, 0); + tree arg1 = CALL_EXPR_ARG (exp, 1); rtx top = expand_expr (arg0, NULL_RTX, ptr_mode, EXPAND_NORMAL); - rtx bot = convert_memory_address (ptr_mode, virtual_stack_dynamic_rtx); + rtx bot = expand_expr (arg1, NULL_RTX, ptr_mode, EXPAND_NORMAL); + rtx off = expand_simple_binop (Pmode, MINUS, virtual_stack_dynamic_rtx, + stack_pointer_rtx, NULL_RTX, 0, + OPTAB_LIB_WIDEN); + off = convert_modes (ptr_mode, Pmode, off, 0); + bot = expand_simple_binop (ptr_mode, PLUS, bot, off, NULL_RTX, 0, + OPTAB_LIB_WIDEN); rtx ret = init_one_libfunc ("__asan_allocas_unpoison"); ret = emit_library_call_value (ret, NULL_RTX, LCT_NORMAL, ptr_mode, top, ptr_mode, bot, ptr_mode);