From patchwork Thu Apr 11 08:38:39 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 1922424 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=F0uq7//O; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4VFY5d2shBz1yYM for ; Thu, 11 Apr 2024 18:39:15 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 1BDE2384AB77 for ; Thu, 11 Apr 2024 08:39:13 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id B2B82385840B for ; Thu, 11 Apr 2024 08:38:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B2B82385840B Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org B2B82385840B Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712824732; cv=none; b=kbMis+WT4xlVWFmLLlQecDmk3RsLeE+x1O/SGF5LY9NBgMz800P8x5L2FYNOPBATTNSzoSh4hxoA4AOmkldiRBQLMKgFyVHUgVNHKCVLsh/irUShF1JgOjZ9mo6+3k2t7FRPb8ZfMUbAXAX1IX2HIPybys76rzHswJFWr7x5Vbk= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712824732; c=relaxed/simple; bh=BqBT9JaK7c208weEdDTvyJ698tGh/YZ+dAvtAxheEo4=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=SyU2FBYe7gb18W5qBKF+dZpoDscgg1ToaaloVETPkBDvEr6/12Ryp3s6ag17Pf6JRTZQ5ip4y3B7PvRO82ODhoMM+XTjSdQlSQM8TW+P7npXDuINsIPPnWxJvxisVIsuCvwBpIAx5p3dwalbg8wYqONI0pZJ2VCsSqPcEtb+ufc= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1712824730; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=bxM3FcbBsBny4X1BFaoe5SkXexZEzOfOoFqNMjeOM6A=; b=F0uq7//OYtvuLy9AQCb3eGVrAAxJd5y7SeAnCG7mOu8Q4sqFg6aTZKTQLyTSK1Oqeizr4G l2heQBuGcLf70Njda8wB8bX9ssK7xyTY2SA/sIad5h/oikIxdlQBEJlUznOnkSRoYXZ6tb TFkE8yuphwtwpgmDm3QuouppLMqfjAQ= Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-423-SC5A3tKuMUidEdw6Ulj1pw-1; Thu, 11 Apr 2024 04:38:46 -0400 X-MC-Unique: SC5A3tKuMUidEdw6Ulj1pw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0E9883803914; Thu, 11 Apr 2024 08:38:46 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.45.224.14]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8ABF32026D06; Thu, 11 Apr 2024 08:38:45 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 43B8cdIg3018210 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 11 Apr 2024 10:38:39 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 43B8cdH43018209; Thu, 11 Apr 2024 10:38:39 +0200 Date: Thu, 11 Apr 2024 10:38:39 +0200 From: Jakub Jelinek To: Richard Biener , Jeff Law , liuhongt Cc: gcc-patches@gcc.gnu.org Subject: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027] Message-ID: References: <20240326060802.3443261-1-hongtao.liu@intel.com> MIME-Version: 1.0 In-Reply-To: <20240326060802.3443261-1-hongtao.liu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Jakub Jelinek Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org On Tue, Mar 26, 2024 at 02:08:02PM +0800, liuhongt wrote: > > > So, try to add some other variable with larger size and smaller alignment > > > to the frame (and make sure it isn't optimized away). > > > > > > alignb above is the alignment of the first partition's var, if > > > align_frame_offset really needs to depend on the var alignment, it probably > > > should be the maximum alignment of all the vars with alignment > > > alignb * BITS_PER_UNIT <=3D MAX_SUPPORTED_STACK_ALIGNMENT > > > > > In asan_emit_stack_protection, when it allocated fake stack, it assume > bottom of stack is also aligned to alignb. And the place violated this > is the first var partition. which is 32 bytes offsets, it should be > BIGGEST_ALIGNMENT / BITS_PER_UNIT. > So I think we need to use MAX (BIGGEST_ALIGNMENT / > BITS_PER_UNIT, ASAN_RED_ZONE_SIZE) for the first var partition. Your first patch aligned offsets[0] to maximum of alignb and ASAN_RED_ZONE_SIZE. But as I wrote in the reply to that mail, alignb there is the alignment of just a single variable which is the first one to appear in the sorted list and is placed in the highest spot in the stack frame. That is not necessarily the largest alignment, the sorting ensures that it is a variable with the largest size in the frame (and only if several of them have equal size, largest alignment from the same sized ones). Your second patch used maximum of BIGGEST_ALIGNMENT / BITS_PER_UNIT and ASAN_RED_ZONE_SIZE. That doesn't change anything at all when using -mno-avx512f - offsets[0] is still just 32-byte aligned in that case relative to top of frame, just changes the -mavx512f case to be 64-byte aligned offsets[0] (aka offsets[0] is then either 0 or -64 instead of either 0 or -32). That will not help if any variable in the frame needs 128-byte, 256-byte, 512-byte ... 4096-byte alignment. If you want to fix the bug in the spot you've touched, you'd need to walk all the stack_vars[stack_vars_sorted[si2]] for si2 [si + 1, n - 1] and for those where the loop would do anything (i.e. stack_vars[i2].representative == i2 && TREE_CODE (decl2) == SSA_NAME ? SA.partition_to_pseudo[var_to_partition (SA.map, decl2)] == NULL_RTX : DECL_RTL (decl2) == pc_rtx and the pred applies (but that means also walking the earlier ones! because with -fstack-protector* the vars can be processed in several calls) and alignb2 * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT and compute maximum of those alignments. That maximum is already computed, data->asan_alignb = MAX (data->asan_alignb, alignb); computes that, but you get the final result only after you do all the expand_stack_vars calls. You'd need to compute it before. Though, that change would be still in the wrong place. The thing is, it would be a waste of the precious stack space when it isn't needed at all (e.g. when asan will not at compile time do the use after return checking, or if it won't do it at runtime, or even if it will do at runtime it will waste the space on the stack). The following patch fixes it solely for the __asan_stack_malloc_N allocations, doesn't enlarge unnecessarily further the actual stack frame. Because asan is only supported on FRAME_GROWS_DOWNWARD architectures (mips, rs6000 and xtensa are conditional FRAME_GROWS_DOWNWARD arches, which for -fsanitize=address or -fstack-protector* use FRAME_GROWS_DOWNWARD 1, otherwise 0, others supporting asan always just use 1), the assumption for the dynamic stack realignment is that the top of the stack frame (aka offset 0) is aligned to alignb passed to the function (which is the maximum of alignb of all the vars in the frame). As checked by the assertion in the patch, offsets[0] is 0 most of the time and so that assumption is correct, the only case when it is not 0 is if -fstack-protector* is on together with -fsanitize=address and cfgexpand.cc (create_stack_guard) created a stack guard. That is the only variable which is allocated in the stack frame right away, for all others with -fsanitize=address defer_stack_allocation (or -fstack-protector*) returns true and so they aren't allocated immediately but handled during the frame layout phases. So, the original frame_offset of 0 is changed because of the stack guard to -pointer_size_in_bytes and later at the if (data->asan_vec.is_empty ()) { align_frame_offset (ASAN_RED_ZONE_SIZE); prev_offset = frame_offset.to_constant (); } to -ASAN_RED_ZONE_SIZE. The asan_emit_stack_protection code wasn't taking this into account though, so essentially assumed in the __asan_stack_malloc_N allocated memory it needs to align it such that pointer corresponding to offsets[0] is alignb aligned. But that isn't correct if alignb > ASAN_RED_ZONE_SIZE, in that case it needs to ensure that pointer corresponding to frame offset 0 is alignb aligned. The following patch fixes that. Unlike the previous case where we knew that asan_frame_size + base_align_bias falls into the same bucket as asan_frame_size, this isn't in some cases true anymore, so the patch recomputes which bucket to use and if going to bucket 11 (because there is no __asan_stack_malloc_11 function in the library) disables the after return sanitization. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2024-04-11 Jakub Jelinek PR middle-end/110027 * asan.cc (asan_emit_stack_protection): Assert offsets[0] is zero if there is no stack protect guard, otherwise -ASAN_RED_ZONE_SIZE. If alignb > ASAN_RED_ZONE_SIZE and there is stack pointer guard, take the ASAN_RED_ZONE_SIZE bytes allocated at the top of the stack into account when computing base_align_bias. Recompute use_after_return_class from asan_frame_size + base_align_bias and set to -1 if that would overflow to 11. * gcc.dg/asan/pr110027.c: New test. Jakub --- gcc/asan.cc.jj 2024-04-10 09:54:39.661231059 +0200 +++ gcc/asan.cc 2024-04-10 12:12:11.337978004 +0200 @@ -1911,19 +1911,39 @@ asan_emit_stack_protection (rtx base, rt } str_cst = asan_pp_string (&asan_pp); + gcc_checking_assert (offsets[0] == (crtl->stack_protect_guard + ? -ASAN_RED_ZONE_SIZE : 0)); /* Emit the prologue sequence. */ if (asan_frame_size > 32 && asan_frame_size <= 65536 && pbase && param_asan_use_after_return) { + HOST_WIDE_INT adjusted_frame_size = asan_frame_size; + /* The stack protector guard is allocated at the top of the frame + and cfgexpand.cc then uses align_frame_offset (ASAN_RED_ZONE_SIZE); + while in that case we can still use asan_frame_size, we need to take + that into account when computing base_align_bias. */ + if (alignb > ASAN_RED_ZONE_SIZE && crtl->stack_protect_guard) + adjusted_frame_size += ASAN_RED_ZONE_SIZE; use_after_return_class = floor_log2 (asan_frame_size - 1) - 5; /* __asan_stack_malloc_N guarantees alignment N < 6 ? (64 << N) : 4096 bytes. */ if (alignb > (use_after_return_class < 6 ? (64U << use_after_return_class) : 4096U)) use_after_return_class = -1; - else if (alignb > ASAN_RED_ZONE_SIZE && (asan_frame_size & (alignb - 1))) - base_align_bias = ((asan_frame_size + alignb - 1) - & ~(alignb - HOST_WIDE_INT_1)) - asan_frame_size; + else if (alignb > ASAN_RED_ZONE_SIZE + && (adjusted_frame_size & (alignb - 1))) + { + base_align_bias + = ((adjusted_frame_size + alignb - 1) + & ~(alignb - HOST_WIDE_INT_1)) - adjusted_frame_size; + use_after_return_class + = floor_log2 (asan_frame_size + base_align_bias - 1) - 5; + if (use_after_return_class > 10) + { + base_align_bias = 0; + use_after_return_class = -1; + } + } } /* Align base if target is STRICT_ALIGNMENT. */ --- gcc/testsuite/gcc.dg/asan/pr110027.c.jj 2024-04-10 12:01:19.939768472 +0200 +++ gcc/testsuite/gcc.dg/asan/pr110027.c 2024-04-10 12:11:52.728229147 +0200 @@ -0,0 +1,50 @@ +/* PR middle-end/110027 */ +/* { dg-do run } */ +/* { dg-additional-options "-fstack-protector-strong" { target fstack_protector } } */ +/* { dg-set-target-env-var ASAN_OPTIONS "detect_stack_use_after_return=1" } */ + +struct __attribute__((aligned (128))) S { char s[128]; }; +struct __attribute__((aligned (64))) T { char s[192]; }; +struct __attribute__((aligned (32))) U { char s[256]; }; +struct __attribute__((aligned (64))) V { char s[320]; }; +struct __attribute__((aligned (128))) W { char s[512]; }; + +__attribute__((noipa)) void +foo (void *p, void *q, void *r, void *s) +{ + if (((__UINTPTR_TYPE__) p & 31) != 0 + || ((__UINTPTR_TYPE__) q & 127) != 0 + || ((__UINTPTR_TYPE__) r & 63) != 0) + __builtin_abort (); + (void *) s; +} + +__attribute__((noipa)) int +bar (void) +{ + struct U u; + struct S s; + struct T t; + char p[4]; + foo (&u, &s, &t, &p); + return 42; +} + +__attribute__((noipa)) int +baz (void) +{ + struct W w; + struct U u; + struct V v; + char p[4]; + foo (&u, &w, &v, &p); + return 42; +} + +int +main () +{ + bar (); + baz (); + return 0; +}