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

From: Vincent MAILHOL
Date: Wed Sep 11 2024 - 00:46:10 EST


On Wed. 11 Sep. 2024 at 09:36, Kees Cook <kees@xxxxxxxxxx> wrote:
> 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] = ...;

To me, this pointer arithmetic is just a bug. Anyone in his right mind
would write:

typeof(*s->fam) *element = s->fam;

When I compare your example to my example, I think that both are
convoluted and unrealistic but with the *huge* difference that:

- in my example, the code logic is correct and the thing breaks
because of the current overestimation in struct_size(). See the
clang documentation:

https://clang.llvm.org/docs/AttributeReference.html#counted-by

The clang guys are using the same calculation as in my patch. So I
can think of a world in which someone with good intent would copy
this example, and then (for example because of a collaborative
work), someone else would use the struct_size() to do a copy or a
memset resulting in an out of bound as illustrated in my example.

- in your example, the code is incorrect. If we start to apply that
"what if users do random pointer arithmetic" reasoning, then
anything can be proven to be a security risk.

To repeat, I think that my example was convoluted, but I see yours as
even more convoluted.

So, with this said, what do you think is the good trade-off? Return
the exact size so that code with correct logic works as intended or
keep the extra bytes in case someone does some crazy pointer
arithmetic?

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

How does a struct with "multiple trailing flexible arrays" look like? By
the C standard, structures can only have one single fam, so this isn't
something I am aware of.

Would the struct_size() macro still be relevant for those "multiple
fam" structures or would the structure size also be hand calculated?
Isn't this argument out of scope of this patch discussion?

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

So far, I think that my example is more concrete than your pointer
arithmetic argument. Not by a lot, and this is why I just put that
security argument at the end after the --- cutter.


Yours sincerely,
Vincent Mailhol