Message-Id: <20240910024952.1590207-1-mailhol.vincent@wanadoo.fr>
Date: Tue, 10 Sep 2024 11:49:52 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Kees Cook <kees@...nel.org>,
	David Laight <David.Laight@...LAB.COM>
Cc: "Gustavo A . R . Silva" <gustavoars@...nel.org>,
	linux-hardening@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Vincent Mailhol <mailhol.vincent@...adoo.fr>
Subject: [PATCH v2] overflow: optimize struct_size() calculation

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@...adoo.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.


Changelog:

  * v1 -> v2:

    - replace max_t() with max()
    - change description:

        the alignment of the fam is less than the penultimate member's
        alignment

      into:

        the alignment of the fam is less than or equal to the
        penultimate member's alignment

    Link: https://lore.kernel.org/linux-hardening/20240909115221.1298010-1-mailhol.vincent@wanadoo.fr/
---
 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..fc6ea220bf17 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(sizeof(*(p)),					\
+		    size_add(offsetof(typeof(*(p)), member),		\
+			     flex_array_size(p, member, count))))
 
 /**
  * struct_size_t() - Calculate size of structure with trailing flexible array
-- 
2.25.1

Powered by blists - more mailing lists