mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] overflow: Support sub-union members in struct_size()
@ 2026-07-28 11:49 Bence Csokas
  2026-07-28 18:57 ` Gustavo A. R. Silva
  2026-08-06 17:24 ` Mark Rutland
  0 siblings, 2 replies; 3+ messages in thread
From: Bence Csokas @ 2026-07-28 11:49 UTC (permalink / raw)
  To: linux-hardening, linux-kernel
  Cc: Bence Csokas, Mark Rutland, Kees Cook, Gustavo A. R. Silva

Original definition of struct_size() assumed that the total size of an
object with a flex-array member can be calculated simply by adding the
flex-array size to what sizeof() returns. This is however not the case
if the struct has a union inside it, and `member` refers to a field
inside that sub-union:

  struct my_inner {
    int a;
    char b[];
  };

  struct my_outer {
    char c;
    union {
      struct my_inner inner;
      long l;
    };
  };

  /* This will give the wrong result */
  struct_size_t(struct my_outer, inner.b, n_elems);

Replacing sizeof() by the offsetof() of the member fixes this.

Existing users specifying non-union members should not be affected.

Cc: Mark Rutland <mark.rutland@arm.com>

Signed-off-by: Bence Csokas <bence.csokas@arm.com>
---
 include/linux/overflow.h | 4 ++--
 include/linux/stddef.h   | 8 ++++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index a8cb6319b4fb..a11c1135f617 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -433,8 +433,8 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
  */
 #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)))
+		offsetofp((p), member) + flex_array_size(p, member, count),	\
+		size_add(offsetofp((p), member), flex_array_size(p, member, count)))
 
 /**
  * struct_size_t() - Calculate size of structure with trailing flexible array
diff --git a/include/linux/stddef.h b/include/linux/stddef.h
index e1851c50c89b..b8f0dbd2de55 100644
--- a/include/linux/stddef.h
+++ b/include/linux/stddef.h
@@ -32,6 +32,14 @@ enum {
 #define offsetofend(TYPE, MEMBER) \
 	(offsetof(TYPE, MEMBER)	+ sizeof_field(TYPE, MEMBER))
 
+/**
+ * offsetofp() - Get the offset of a struct field from a pointer to the struct
+ *
+ * @P: Pointer to the structure
+ * @MEMBER: The member within the structure to get the end offset of
+ */
+#define offsetofp(P, MEMBER) offsetof(typeof(*(P)), MEMBER)
+
 /**
  * struct_group() - Wrap a set of declarations in a mirrored struct
  *

base-commit: 62cc90241548d5570ee68e01aaba6506964e9811
-- 
2.54.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] overflow: Support sub-union members in struct_size()
  2026-07-28 11:49 [PATCH] overflow: Support sub-union members in struct_size() Bence Csokas
@ 2026-07-28 18:57 ` Gustavo A. R. Silva
  2026-08-06 17:24 ` Mark Rutland
  1 sibling, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2026-07-28 18:57 UTC (permalink / raw)
  To: Bence Csokas, linux-hardening, linux-kernel
  Cc: Mark Rutland, Kees Cook, Gustavo A. R. Silva



On 7/28/26 05:49, Bence Csokas wrote:
> Original definition of struct_size() assumed that the total size of an
> object with a flex-array member can be calculated simply by adding the
> flex-array size to what sizeof() returns. This is however not the case
> if the struct has a union inside it, and `member` refers to a field
> inside that sub-union:
> 
>    struct my_inner {
>      int a;
>      char b[];
>    };
> 
>    struct my_outer {
>      char c;
>      union {
>        struct my_inner inner;
>        long l;
>      };
>    };
> 
>    /* This will give the wrong result */
>    struct_size_t(struct my_outer, inner.b, n_elems);
> 
> Replacing sizeof() by the offsetof() of the member fixes this.
> 
> Existing users specifying non-union members should not be affected.

This actually affects users of flexible structs that have tail padding[1].

-Gustavo

[1] https://godbolt.org/z/KPq59EYej

> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> 
> Signed-off-by: Bence Csokas <bence.csokas@arm.com>
> ---
>   include/linux/overflow.h | 4 ++--
>   include/linux/stddef.h   | 8 ++++++++
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index a8cb6319b4fb..a11c1135f617 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -433,8 +433,8 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
>    */
>   #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)))
> +		offsetofp((p), member) + flex_array_size(p, member, count),	\
> +		size_add(offsetofp((p), member), flex_array_size(p, member, count)))
>   
>   /**
>    * struct_size_t() - Calculate size of structure with trailing flexible array
> diff --git a/include/linux/stddef.h b/include/linux/stddef.h
> index e1851c50c89b..b8f0dbd2de55 100644
> --- a/include/linux/stddef.h
> +++ b/include/linux/stddef.h
> @@ -32,6 +32,14 @@ enum {
>   #define offsetofend(TYPE, MEMBER) \
>   	(offsetof(TYPE, MEMBER)	+ sizeof_field(TYPE, MEMBER))
>   
> +/**
> + * offsetofp() - Get the offset of a struct field from a pointer to the struct
> + *
> + * @P: Pointer to the structure
> + * @MEMBER: The member within the structure to get the end offset of
> + */
> +#define offsetofp(P, MEMBER) offsetof(typeof(*(P)), MEMBER)
> +
>   /**
>    * struct_group() - Wrap a set of declarations in a mirrored struct
>    *
> 
> base-commit: 62cc90241548d5570ee68e01aaba6506964e9811


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] overflow: Support sub-union members in struct_size()
  2026-07-28 11:49 [PATCH] overflow: Support sub-union members in struct_size() Bence Csokas
  2026-07-28 18:57 ` Gustavo A. R. Silva
@ 2026-08-06 17:24 ` Mark Rutland
  1 sibling, 0 replies; 3+ messages in thread
From: Mark Rutland @ 2026-08-06 17:24 UTC (permalink / raw)
  To: Bence Csokas
  Cc: linux-hardening, linux-kernel, Kees Cook, Gustavo A. R. Silva

Hi Bence,

As discussed elsewhere, I don't think this is right, but it really
depends on how struct_size_t() is expected to be used. We probably need
better documentation for that...

On Tue, Jul 28, 2026 at 01:49:13PM +0200, Bence Csokas wrote:
> Original definition of struct_size() assumed that the total size of an
> object with a flex-array member can be calculated simply by adding the
> flex-array size to what sizeof() returns. This is however not the case
> if the struct has a union inside it, and `member` refers to a field
> inside that sub-union:
> 
>   struct my_inner {
>     int a;
>     char b[];
>   };
> 
>   struct my_outer {
>     char c;
>     union {
>       struct my_inner inner;
>       long l;
>     };
>   };
> 
>   /* This will give the wrong result */
>   struct_size_t(struct my_outer, inner.b, n_elems);

Can you please explain what's wrong specifically? How are you expecting
to use the result?

My understanding is that struct_size_t() is meant to be used to
determine the size to allocate. Over-estimating by a small margin isn't
a functional issue.

> Replacing sizeof() by the offsetof() of the member fixes this.

That will under-estimate the size of the structure, by ignoring the
static footprint. For your example above, imagine you called:

  struct_size_t(struct my_outer, inner.b, 1) 

Where sizeof(long) == 8, that will provide a result that *smaller* than
sizeof(struct my_outer), missing the last 3 bytes of 'l'.

It's possible that 'l' is the active member at some point in time.  At
allocation we need to allocate *the largest possible size* that will be
used, but we're not guaranteeing that the flexible member is alway used.

Consider a case with a tagged union, where another field is used to
determine the active union member.

For example, something like:

| struct foo_elem {
|	/* some small set of fields */
| 	...
| };
| 
| struct source {
|	/* some large set of fields */
| 	...
| };
| 
| struct foo {
| 	int	nr_elems;
| 	bool	completed;
| 
| 	/* 
| 	 * When !completed, use @source.
| 	 * When completed, results have been placed in @elems.
| 	 */
| 	union {
| 		struct source source;
| 		struct foo_elem elems[];
| 	};
| };

The current behaviour for struct_size[_t]() will ensure sufficient space
is always allocated, whereas what you proposed might not. That could
lead to memory corruption, etc, which would be a worse than
over-allocating.

Mark.

> Existing users specifying non-union members should not be affected.
> 
> Cc: Mark Rutland <mark.rutland@arm.com>
> 
> Signed-off-by: Bence Csokas <bence.csokas@arm.com>
> ---
>  include/linux/overflow.h | 4 ++--
>  include/linux/stddef.h   | 8 ++++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index a8cb6319b4fb..a11c1135f617 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -433,8 +433,8 @@ static __always_inline size_t __must_check size_sub(size_t minuend, size_t subtr
>   */
>  #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)))
> +		offsetofp((p), member) + flex_array_size(p, member, count),	\
> +		size_add(offsetofp((p), member), flex_array_size(p, member, count)))
>  
>  /**
>   * struct_size_t() - Calculate size of structure with trailing flexible array
> diff --git a/include/linux/stddef.h b/include/linux/stddef.h
> index e1851c50c89b..b8f0dbd2de55 100644
> --- a/include/linux/stddef.h
> +++ b/include/linux/stddef.h
> @@ -32,6 +32,14 @@ enum {
>  #define offsetofend(TYPE, MEMBER) \
>  	(offsetof(TYPE, MEMBER)	+ sizeof_field(TYPE, MEMBER))
>  
> +/**
> + * offsetofp() - Get the offset of a struct field from a pointer to the struct
> + *
> + * @P: Pointer to the structure
> + * @MEMBER: The member within the structure to get the end offset of
> + */
> +#define offsetofp(P, MEMBER) offsetof(typeof(*(P)), MEMBER)
> +
>  /**
>   * struct_group() - Wrap a set of declarations in a mirrored struct
>   *
> 
> base-commit: 62cc90241548d5570ee68e01aaba6506964e9811
> -- 
> 2.54.0
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-08-06 17:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-07-28 11:49 [PATCH] overflow: Support sub-union members in struct_size() Bence Csokas
2026-07-28 18:57 ` Gustavo A. R. Silva
2026-08-06 17:24 ` Mark Rutland

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®