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

[Kees Cook <keescook@chromium.org>]

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.


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)

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))

-- 
Kees Cook