Re: [PATCH v2] overflow: optimize struct_size() calculation

[Kees Cook]

On Tue, Sep 10, 2024 at 11:49:52AM +0900, Vincent Mailhol wrote:
> If the offsetof() of a given flexible array member (fam) is smaller
> than the sizeof() of the containing struct, then the struct_size()
> macro reports a size which is too big.
> 
> This occurs when the two conditions below are met:
> 
>   - there are padding bytes after the penultimate member (the member
>     preceding the fam)
>   - the alignment of the fam is less than or equal to the penultimate
>     member's alignment
> 
> In that case, the fam overlaps with the padding bytes of the
> penultimate member. This behaviour is not captured in the current
> struct_size() macro, potentially resulting in an overestimated size.
> 
> Below example illustrates the issue:
> 
>   struct s {
>   	u64 foo;
>   	u32 count;
>   	u8 fam[] __counted_by(count);
>   };
> 
> Assuming a 64 bits architecture:
> 
>   - there are 4 bytes of padding after s.count (the penultimate
>     member)
>   - sizeof(struct s) is 16 bytes
>   - the offset of s.fam is 12 bytes
>   - the alignment of s.fam is 1 byte
> 
> The sizes are as below:
> 
>    s.count	current struct_size()	actual size
>   -----------------------------------------------------------------------
>    0		16			16
>    1		17			16
>    2		18			16
>    3		19			16
>    4		20			16
>    5		21			17
>    .		.			.
>    .		.			.
>    .		.			.
>    n		sizeof(struct s) + n	max(sizeof(struct s),
> 					    offsetof(struct s, fam) + n)
> 
> Change struct_size() from this pseudo code logic:
> 
>   sizeof(struct s) + sizeof(*s.fam) * s.count
> 
> to that pseudo code logic:
> 
>   max(sizeof(struct s), offsetof(struct s, fam) + sizeof(*s.fam) * s.count)
> 
> Here, the lowercase max*() macros can cause struct_size() to return a
> non constant integer expression which would break the DEFINE_FLEX()
> macro by making it declare a variable length array. Because of that,
> use the unsafe MAX() macro only if the expression is constant and use
> the safer max() otherwise.
> 
> Reference: ISO/IEC 9899:2018 §6.7.2.1 "Structure and union specifiers" ¶18
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> ---
> 
> I also tried to think of whether the current struct_size() macro could
> be a security issue.
> 
> The only example I can think of is if someone manually allocates the
> exact size but then use the current struct_size() macro.
> 
> For example (reusing the struct s from above):
> 
>   u32 count = 5;
>   struct s *s = kalloc(offsetof(typeof(*s), fam) + count);
>   s->count = count;
>   memset(s, 0, struct_size(s, fam, count)); /* 4 bytes buffer overflow */
> 
> If we have concerns that above pattern may actually exist, then this
> patch should also go to stable. I personally think that the above is a
> bit convoluted and, as such, I only suggest this patch to go to next.

Yeah, this "over-estimation" has come up before, and my thinking as been
that while the above situation is certainly possible (but unlikely),
I've worried that the reverse situation (after this patch) is
potentially worse, where we allocate very precisely, but then manually
index too far:

	struct s *s = kmalloc(struct_size(s, fam, count), gfp);
	typeof(*s->fam) *element;
	element = (void *)s + sizeof(*s);
	for (i = 0; i < count; i++)
		element[i] = ...;

And for a max 7 byte savings, I'm concerned we can get bit much worse in
the above situation. It *should* be unlikely, but I've especially seen a
lot of manually calculated games especially for structs that have
effectively multiple trailing flexible arrays (wifi likes to do this,
for example).

So while I don't have very concrete evidence, my sensation is that we're
in a more defensive position leaving it over-estimated.

-- 
Kees Cook