Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

[Kees Cook <keescook@chromium.org>]

On Tue, Aug 08, 2023 at 04:54:46PM +0200, Martin Uecker wrote:
> 
> 
> I am sure this has been discussed before, but seeing that you
> test for a specific formula, let me point out the following:
> 
> There at least three different size expression which could
> make sense. Consider
> 
> short foo { int a; short b; char t[]; }; 
> 
> Most people seem to use
> 
> sizeof(struct foo) + N * sizeof(foo->t);

I think this contains a typo. The above should be:

	sizeof(struct foo) + N * sizeof(*foo->t);

> which for N == 3 yields 11 bytes on x86-64 because the formula
> adds the padding of the original struct. There is an example
> in the  C standard that uses this formula.
> 
> 
> But he minimum size of an object which stores N elements is
> 
> max(sizeof (struct s), offsetof(struct s, t[n]))
> 
> which is 9 bytes. 
> 
> This is what clang uses for statically allocated objects with
> initialization, while GCC uses the rule above (11 bytes). But 
> bdos / bos  then returns the smaller size of 9 which is a bit
> confusing.
> 
> https://godbolt.org/z/K1hvaK1ns

And to add to the mess here, Clang used to get this wrong for statically
initialized flexible array members (returning 8):

https://godbolt.org/z/Tee96dazs

But that was fixed recently when I noticed it a few weeks ago.

> https://github.com/llvm/llvm-project/issues/62929
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109956
> 
> 
> Then there is also the size of a similar array where the FAM
> is replaced with an array of static size:
> 
> struct foo { int a; short b; char t[3]; }; 
> 
> This would make the most sense to me, but it has 12 bytes
> because the padding is according to the usual alignment
> rules.

Hm, in my thinking, FAMs are individually padded, so since they don't
"count" to the struct size, I think this is "as expected".

Note that the Linux kernel explicitly chooses to potentially over-estimate
for FAM allocations since there has been inconsistent sizes used depend
on the style of the prior fake FAM usage, the use of unions, etc.

i.e. our macro is, effectively:

#define struct_size(ptr, member, count)	\
	(sizeof(*ptr) + (count) * sizeof(*ptr->member))

since the other case can lead to truncated sizes:

#define struct_size(ptr, member, count) \
	(offsetof(typeof(*ptr), member) + (count) * sizeof(*ptr->member))


struct foo {
	int a, b, c;
	int count;
	union {
		struct {
			char small;
			char char_fam[];
		};
		struct {
			int large;
			int int_fam[];
		};
	};
};

https://godbolt.org/z/b1v74Mzhd

i.e.:
struct_size(x, char_fam, 1) < sizeof(*x)

so accessing "large" fails, etc. Yes, it's all horrible, but we have to
deal with this kind of thing in the kernel. :(

-- 
Kees Cook