From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 77F94385840A for ; Fri, 29 Jul 2022 06:20:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 77F94385840A Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 061411FCE0; Fri, 29 Jul 2022 06:20:53 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id C5D142C141; Fri, 29 Jul 2022 06:20:52 +0000 (UTC) Date: Fri, 29 Jul 2022 06:20:52 +0000 (UTC) From: Richard Biener To: Kees Cook cc: Qing Zhao , gcc-patches Paul A Clarke via , jakub Jelinek , martin Sebor , "joseph@codesourcery.com" Subject: Re: [GCC13][Patch][V2][1/2]Add a new option -fstrict-flex-array[=n] and attribute strict_flex_array(n) and use it in PR101836 In-Reply-To: <202207282202.88A5A9E8@keescook> Message-ID: References: <202207282202.88A5A9E8@keescook> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_NUMSUBJECT, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no 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.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Jul 2022 06:20:57 -0000 On Thu, 28 Jul 2022, Kees Cook wrote: > On Thu, Jul 28, 2022 at 07:26:57AM +0000, Richard Biener wrote: > > On Tue, 19 Jul 2022, Qing Zhao wrote: > > > [...] > > > +@cindex @code{strict_flex_array} variable attribute > > > +@item strict_flex_array (@var{level}) > > > +The @code{strict_flex_array} attribute should be attached to the trailing > > > +array field of a structure. It specifies the level of strictness of > > > +treating the trailing array field of a structure as a flexible array > > > +member. @var{level} must be an integer betwen 0 to 3. > > > + > > > +@var{level}=0 is the least strict level, all trailing arrays of structures > > > +are treated as flexible array members. @var{level}=3 is the strictest level, > > > +only when the trailing array is declared as a flexible array member per C99 > > > +standard onwards ([]), it is treated as a flexible array member. > > > > How is level 3 (thus -fstrict-flex-array) interpreted when you specify > > -std=c89? How for -std=gnu89? > > To me, it makes sense that either c99 is required (most sane to me) > or it would disable flexible arrays entirely (seems an unlikely combo to > be useful). > > > > > > + > > > +There are two more levels in between 0 and 3, which are provided to support > > > +older codes that use GCC zero-length array extension ([0]) or one-size array > > > +as flexible array member ([1]): > > > +When @var{level} is 1, the trailing array is treated as a flexible array member > > > +when it is declared as either "[]", "[0]", or "[1]"; > > > +When @var{level} is 2, the trailing array is treated as a flexible array member > > > +when it is declared as either "[]", or "[0]". > > > > Given the above does adding level 2 make sense given that [0] is a GNU > > extension? > > Level 1 removes the general "all trailing arrays are flex arrays" logic, but > allows the 2 common "historical" fake flex array styles ("[1]" and "[0]"). > Level 2 additionally removes the "[1]" style. > Level 3 additionally removes the "[0]" style. > > I don't understand how "[0]" being a GNU extension matters here for > level 2 -- it's dropping "[1]". And for level 3, the point is to defang > the GNU extension of "[0]" to no longer mean "flexible array", and > instead only mean "zero sized member" (as if it were something like > "struct { } no_size;"). > > Note that for the Linux kernel, we only care about level 3, but could > make do with level 2. We need to purge all the "fake" flexible array usage > so we can start building a sane set of behaviors around array bounds > that are reliably introspectable. Note we've seen "historical" fake flex arrays like struct X { int n; char str[4]; }; used by people being extra clever (or careful? char str[1] would not be a flex array since there's a padding "member" behind it?!) in handling the padding. I was just worried in confusing people too much. Given -fstrict-flex-arrays enables level 3 should we warn with -std=c89 that it disables all flex arrays? I think it should at least be documented somehow. > As a related bit of feature creep, it would be great to expose something > like __builtin_has_flex_array_p() so FORTIFY could do a better job > filtering __builtin_object_size() information. > > Given: > > struct inside { > int foo; > int bar; > unsigned long items[]; > }; > > struct outside { > int a; > int b; > struct inside inner; > }; > > The follow properties are seen within, for example: > > void stuff(struct outside *outer, struct inside *inner) > { > ... > } > > __builtin_object_size(&outer->inner, 1) == 8 > __builtin_object_size(inner, 1) == -1 > > (see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832) I think that would be a bug in bos worth fixing. > So things like FORTIFY misfire on &outer->inner, as it's _not_ actually > 8 bytes -- it has a potential trailing flex array. > > If it could be introspected better, FORTIFY could check for the flex > array. For example, instead of using the inconsistent __bos(ptr, 1) for > finding member sizes, it could do something like: > > #define __member_size(ptr) \ > (__builtin_has_flex_array_p(ptr) ? -1 : \ > __builtin_object_size(ptr, 1)) > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)