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

[Vincent MAILHOL]

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