From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.223.131]) by sourceware.org (Postfix) with ESMTPS id 0D01F385841F for ; Thu, 11 Apr 2024 08:53:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0D01F385841F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 0D01F385841F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.223.131 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712825613; cv=none; b=iv68NFWNiptD0wzZa4rMwDL4+UMJnKQE4lcTbigSPff+P8dccLkDhzchZ5nqFKgcsbLxiL1JRPwZ5Q7L5NYlJ/2fLCTlz7MMObqC/cUqJj3Noc2pBxrSmzTaCyQhKnfetb96a3hdk+V/So0ffNI3jT6CKdHrclf3m0dkp738yQI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712825613; c=relaxed/simple; bh=zqsjzH+q1ZdrxbfOeaiq1AExWIwjXfFAewQPJf3gmqE=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature:Date: From:To:Subject:Message-ID:MIME-Version; b=ZcdTH39wSQRMfx3gAZGViyHaVh6JKfnAPk2WqcmUlv1vtNDi6H6/n+w8LJMpX5OvtRDcbAtag1BZvfkE5kWxgNIw49ayHxtRSrQ89BuXLHc1QcBkjV75nUg00uh/3SVfyUSscBQk1OqbH7L5o7swwhrEKKobQxNmLmq7mQZxEOE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [10.168.5.241] (unknown [10.168.5.241]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 70E0E20CDC; Thu, 11 Apr 2024 08:53:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1712825609; h=from:from:reply-to: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=QjmfZoM+gRtr/5qYncB5Y9lMu8fi+axxGV3CDdFU8kE=; b=F+HBIKtGfZvb8vT0ogtC5WkFH/YHgQCwMFUzrJG+EenCqiLYlhLGyVrOI1QToa+LJK+ZxI bJLqCbxShvDvUUt2hWD7g74CTnQBo8ls2eqYz7VobM6+8WepVlcWhHB4xEm2TUNnmb4/hJ v/Gbh4XvzOQWVEbqeO9bo/RJ2BmD8lI= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1712825609; h=from:from:reply-to: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=QjmfZoM+gRtr/5qYncB5Y9lMu8fi+axxGV3CDdFU8kE=; b=QUVIkMsuvdPGuY0xgggTamc+hrgnmMFe/Om9A3DbC1qpThuK9bzgRhVMbZcjRuS0Z8ppTA Z4w0+ELMl7UjNUDg== Authentication-Results: smtp-out2.suse.de; none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1712825608; h=from:from:reply-to: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=QjmfZoM+gRtr/5qYncB5Y9lMu8fi+axxGV3CDdFU8kE=; b=rd9G3C8xkH4uWppeHXsro8IIc0tAgItZIyYQUqtxQ4n1aR0Jqsdf4XFu5CuP+V6f2slqTm ziWxbv6+wjAY5OWAd94n2WsHT6RQylLq+Sx1vyb/2mI934yjWmBsctuNnYnqILOb9Av3SZ QeYiJbp7DtVlkfjfNuuL5bq+yxHDhBw= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1712825608; h=from:from:reply-to: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=QjmfZoM+gRtr/5qYncB5Y9lMu8fi+axxGV3CDdFU8kE=; b=PT7jtb58rRBEiPdLkQKd9sdT4moWgxkyfv+grl1j2z7A7BUDdV7pa1cGH4sFq8leNym5uA FY61zPYCz7ex7IDQ== Date: Thu, 11 Apr 2024 10:53:28 +0200 (CEST) From: Richard Biener To: Jakub Jelinek cc: Jeff Law , liuhongt , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] asan, v3: Fix up handling of > 32 byte aligned variables with -fsanitize=address -fstack-protector* [PR110027] In-Reply-To: Message-ID: <2sq514qs-p618-s168-2p7o-9s1597078pq2@fhfr.qr> References: <20240326060802.3443261-1-hongtao.liu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Level: X-Spamd-Result: default: False [-4.30 / 50.00]; BAYES_HAM(-3.00)[100.00%]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.20)[-0.999]; MIME_GOOD(-0.10)[text/plain]; ARC_NA(0.00)[]; TO_DN_SOME(0.00)[]; MISSING_XM_UA(0.00)[]; RCVD_COUNT_ZERO(0.00)[0]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVRCPT(0.00)[gmail.com]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; FROM_HAS_DN(0.00)[]; FREEMAIL_CC(0.00)[gmail.com,intel.com,gcc.gnu.org]; RCPT_COUNT_THREE(0.00)[4]; FROM_EQ_ENVFROM(0.00)[]; FUZZY_BLOCKED(0.00)[rspamd.com]; TO_MATCH_ENVRCPT_ALL(0.00)[]; DBL_BLOCKED_OPENRESOLVER(0.00)[cfgexpand.cc:url,asan.cc:url,suse.de:email] X-Spam-Score: -4.30 X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,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 List-Id: On Thu, 11 Apr 2024, Jakub Jelinek wrote: > 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? LGTM. Thanks, Richard. > 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. > > --- 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; > +} > > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)