From: Vincent Mailhol
> Sent: 09 September 2024 12:52
>
> 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 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)
I actually suspect that it matters so infrequently it isn't worth the effort.
Not only do you need a structure with 'tail padding' but you also need
the size to go from one kmalloc() bucket to another.
>
> 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_t() otherwise.
>
> Reference: ISO/IEC 9899:2018 §6.7.2.1 "Structure and union specifiers" ¶18
>
> Signed-off-by: Vincent Mailhol <mailhol.vinc...@wanadoo.fr>
> ---
>
> 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.
> ---
> include/linux/overflow.h | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 0c7e3dcfe867..1384887f3684 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -5,6 +5,7 @@
> #include <linux/compiler.h>
> #include <linux/limits.h>
> #include <linux/const.h>
> +#include <linux/minmax.h>
>
> /*
> * We need to compute the minimum and maximum values representable in a given
> @@ -369,8 +370,12 @@ static inline size_t __must_check size_sub(size_t
> minuend, size_t subtrahend)
> */
> #define struct_size(p, member, count)
> \
> __builtin_choose_expr(__is_constexpr(count), \
> - sizeof(*(p)) + flex_array_size(p, member, count), \
> - size_add(sizeof(*(p)), flex_array_size(p, member, count)))
> + MAX(sizeof(*(p)), \
> + offsetof(typeof(*(p)), member) + \
> + flex_array_size(p, member, count)), \
> + max_t(size_t, sizeof(*(p)), \
> + size_add(offsetof(typeof(*(p)), member), \
> + flex_array_size(p, member, count))))
I don't think you need max_t() here, a plain max() should suffice.
David
>
> /**
> * struct_size_t() - Calculate size of structure with trailing flexible array
> --
> 2.25.1
>
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
