[PATCH v2] overflow.h: Add flex_array_size() helper

[PATCH v2] overflow.h: Add flex_array_size() helper

[Gustavo A. R. Silva]

Add flex_array_size() helper for the calculation of the size, in bytes,
of a flexible array member contained within an enclosing structure.

Example of usage:

struct something {
	size_t count;
	struct foo items[];
};

struct something *instance;

instance = kmalloc(struct_size(instance, items, count), GFP_KERNEL);
instance->count = count;

memcpy(instance->items, source, flex_array_size(instance, items, instance->count));

The helper returns SIZE_MAX on overflow instead of wrapping around.

(Additionally replace parameter n with count in struct_size() for
unification).

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
Changes in v2:
 - Add further information to the helper documentation.
 - Use same code style as struct_size() for consistency.

 include/linux/overflow.h | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 659045046468f..d2329a914304c 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -304,16 +304,33 @@ static inline __must_check size_t __ab_c_size(size_t a, size_t b, size_t c)
  * struct_size() - Calculate size of structure with trailing array.
  * @p: Pointer to the structure.
  * @member: Name of the array member.
- * @n: Number of elements in the array.
+ * @count: Number of elements in the array.
  *
  * Calculates size of memory needed for structure @p followed by an
- * array of @n @member elements.
+ * array of @count @member elements.
  *
  * Return: number of bytes needed or SIZE_MAX on overflow.
  */
-#define struct_size(p, member, n)					\
-	__ab_c_size(n,							\
+#define struct_size(p, member, count)					\
+	__ab_c_size(count,							\
 		    sizeof(*(p)->member) + __must_be_array((p)->member),\
 		    sizeof(*(p)))
 
+/**
+ * flex_array_size() - Calculate size, in bytes, of a flexible array member
+ * within an enclosing structure. Read on for more details.
+ *
+ * @p: Pointer to the structure.
+ * @member: Name of the flexible array member.
+ * @count: Number of elements in the array.
+ *
+ * Calculates size, in bytes, of a flexible array @member of @count elements
+ * within structure @p.
+ *
+ * Return: number of bytes needed or SIZE_MAX on overflow.
+ */
+#define flex_array_size(p, member, count)					\
+	array_size(count,							\
+		    sizeof(*(p)->member) + __must_be_array((p)->member))
+
 #endif /* __LINUX_OVERFLOW_H */
-- 
2.27.0

Re: [PATCH v2] overflow.h: Add flex_array_size() helper

[Joe Perches]

On Mon, 2020-06-08 at 20:22 -0500, Gustavo A. R. Silva wrote:
> Add flex_array_size() helper for the calculation of the size, in bytes,
> of a flexible array member contained within an enclosing structure.
[]
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
[]
> +/**
> + * flex_array_size() - Calculate size, in bytes, of a flexible array member
> + * within an enclosing structure. Read on for more details.

IMO: "Read on for more details" isn't useful here.
Perhaps better would be something like:

 * flex_array_size() - size of a flexible array (sizeof(typeof(member)) * count)

> + *
> + * @p: Pointer to the structure.
> + * @member: Name of the flexible array member.
> + * @count: Number of elements in the array.
> + *
> + * Calculates size, in bytes, of a flexible array @member of @count elements

IMO: "in bytes, " is redundant.  size is always bytes.

> + * within structure @p.
> + *
> + * Return: number of bytes needed or SIZE_MAX on overflow.
> + */
> +#define flex_array_size(p, member, count)					\
> +	array_size(count,							\
> +		    sizeof(*(p)->member) + __must_be_array((p)->member))
> +
>  #endif /* __LINUX_OVERFLOW_H */

Re: [PATCH v2] overflow.h: Add flex_array_size() helper

[Kees Cook]

On Mon, Jun 08, 2020 at 11:47:03PM -0700, Joe Perches wrote:
> On Mon, 2020-06-08 at 20:22 -0500, Gustavo A. R. Silva wrote:
> > Add flex_array_size() helper for the calculation of the size, in bytes,
> > of a flexible array member contained within an enclosing structure.
> []
> > diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> []
> > +/**
> > + * flex_array_size() - Calculate size, in bytes, of a flexible array member
> > + * within an enclosing structure. Read on for more details.
> 
> IMO: "Read on for more details" isn't useful here.
> Perhaps better would be something like:
> 
>  * flex_array_size() - size of a flexible array (sizeof(typeof(member)) * count)
> 
> > + *
> > + * @p: Pointer to the structure.
> > + * @member: Name of the flexible array member.
> > + * @count: Number of elements in the array.
> > + *
> > + * Calculates size, in bytes, of a flexible array @member of @count elements
> 
> IMO: "in bytes, " is redundant.  size is always bytes.

While yes, that's the expected unit, I don't mind the clarification
given that we want to be distinct from "count" and the size of an
individual array element.

-- 
Kees Cook

Re: [PATCH v2] overflow.h: Add flex_array_size() helper

[Kees Cook]

On Mon, Jun 08, 2020 at 08:22:33PM -0500, Gustavo A. R. Silva wrote:
> Add flex_array_size() helper for the calculation of the size, in bytes,
> of a flexible array member contained within an enclosing structure.
> 
> Example of usage:
> 
> struct something {
> 	size_t count;
> 	struct foo items[];
> };
> 
> struct something *instance;
> 
> instance = kmalloc(struct_size(instance, items, count), GFP_KERNEL);
> instance->count = count;
> 
> memcpy(instance->items, source, flex_array_size(instance, items, instance->count));
> 
> The helper returns SIZE_MAX on overflow instead of wrapping around.
> 
> (Additionally replace parameter n with count in struct_size() for
> unification).
> 
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
> Changes in v2:
>  - Add further information to the helper documentation.
>  - Use same code style as struct_size() for consistency.
> 
>  include/linux/overflow.h | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/overflow.h b/include/linux/overflow.h
> index 659045046468f..d2329a914304c 100644
> --- a/include/linux/overflow.h
> +++ b/include/linux/overflow.h
> @@ -304,16 +304,33 @@ static inline __must_check size_t __ab_c_size(size_t a, size_t b, size_t c)
>   * struct_size() - Calculate size of structure with trailing array.
>   * @p: Pointer to the structure.
>   * @member: Name of the array member.
> - * @n: Number of elements in the array.
> + * @count: Number of elements in the array.
>   *
>   * Calculates size of memory needed for structure @p followed by an
> - * array of @n @member elements.
> + * array of @count @member elements.
>   *
>   * Return: number of bytes needed or SIZE_MAX on overflow.
>   */
> -#define struct_size(p, member, n)					\
> -	__ab_c_size(n,							\
> +#define struct_size(p, member, count)					\
> +	__ab_c_size(count,							\
>  		    sizeof(*(p)->member) + __must_be_array((p)->member),\
>  		    sizeof(*(p)))

I like the "count" change -- "n" can be seen as either count or bytes,
so I like this being distinctly "count".

>  
> +/**
> + * flex_array_size() - Calculate size, in bytes, of a flexible array member
> + * within an enclosing structure. Read on for more details.
> + *
> + * @p: Pointer to the structure.
> + * @member: Name of the flexible array member.
> + * @count: Number of elements in the array.
> + *
> + * Calculates size, in bytes, of a flexible array @member of @count elements
> + * within structure @p.
> + *
> + * Return: number of bytes needed or SIZE_MAX on overflow.
> + */
> +#define flex_array_size(p, member, count)					\
> +	array_size(count,							\
> +		    sizeof(*(p)->member) + __must_be_array((p)->member))
> +
>  #endif /* __LINUX_OVERFLOW_H */

I like it! You mentioned off-list that maybe this could be named
sizeof_flex_array() (like sizeof_field(), etc), and that does seem
attractive. As you also mentioned, it begs the question of renaming
struct_size() to sizeof_struct().

Looking back through the thread[1], it seems the name came from Linus[2],
and was more related to the existing array_size() helper.

So, how about this, as a convention we can use to make a choice:

For things that are strictly constant in size, we can use sizeof_*. For
things that have a dynamic component, we'll use *_size(). So, this patch
is correct as-is.

Acked-by: Kees Cook <keescook@chromium.org>

(I wonder who's tree this should go via?)

-Kees

[1] https://lore.kernel.org/lkml/20180507113902.GC18116@bombadil.infradead.org/
[2] https://lore.kernel.org/lkml/CA+55aFy8DSRoUvtiuu5w+XGOK6tYvtJGBH-i8i-y7aiUD2EGLA@mail.gmail.com/

-- 
Kees Cook

Re: [PATCH v2] overflow.h: Add flex_array_size() helper

[Gustavo A. R. Silva]

On 6/10/20 16:38, Kees Cook wrote:

>> -#define struct_size(p, member, n)					\
>> -	__ab_c_size(n,							\
>> +#define struct_size(p, member, count)					\
>> +	__ab_c_size(count,							\
>>  		    sizeof(*(p)->member) + __must_be_array((p)->member),\
>>  		    sizeof(*(p)))
> 
> I like the "count" change -- "n" can be seen as either count or bytes,
> so I like this being distinctly "count".
> 

Yep. :)

>>  
>> +/**
>> + * flex_array_size() - Calculate size, in bytes, of a flexible array member
>> + * within an enclosing structure. Read on for more details.
>> + *
>> + * @p: Pointer to the structure.
>> + * @member: Name of the flexible array member.
>> + * @count: Number of elements in the array.
>> + *
>> + * Calculates size, in bytes, of a flexible array @member of @count elements
>> + * within structure @p.
>> + *
>> + * Return: number of bytes needed or SIZE_MAX on overflow.
>> + */
>> +#define flex_array_size(p, member, count)					\
>> +	array_size(count,							\
>> +		    sizeof(*(p)->member) + __must_be_array((p)->member))
>> +
>>  #endif /* __LINUX_OVERFLOW_H */
> 
> I like it! You mentioned off-list that maybe this could be named
> sizeof_flex_array() (like sizeof_field(), etc), and that does seem
> attractive. As you also mentioned, it begs the question of renaming
> struct_size() to sizeof_struct().
> 
> Looking back through the thread[1], it seems the name came from Linus[2],
> and was more related to the existing array_size() helper.
> 
> So, how about this, as a convention we can use to make a choice:
> 
> For things that are strictly constant in size, we can use sizeof_*. For
> things that have a dynamic component, we'll use *_size(). So, this patch
> is correct as-is.
> 

I like the idea. I haven't thought it in terms of dynamic and constant size,
but it sounds sensible.

> Acked-by: Kees Cook <keescook@chromium.org>
> 
> (I wonder who's tree this should go via?)
> 

Yours? :)

> -Kees
> 
> [1] https://lore.kernel.org/lkml/20180507113902.GC18116@bombadil.infradead.org/
> [2] https://lore.kernel.org/lkml/CA+55aFy8DSRoUvtiuu5w+XGOK6tYvtJGBH-i8i-y7aiUD2EGLA@mail.gmail.com/
> 

--
Gustavo

Re: [PATCH v2] overflow.h: Add flex_array_size() helper

[Kees Cook]

On Tue, Jun 16, 2020 at 03:48:51PM -0500, Gustavo A. R. Silva wrote:
> 
> 
> On 6/10/20 16:38, Kees Cook wrote:
> 
> >> -#define struct_size(p, member, n)					\
> >> -	__ab_c_size(n,							\
> >> +#define struct_size(p, member, count)					\
> >> +	__ab_c_size(count,							\
> >>  		    sizeof(*(p)->member) + __must_be_array((p)->member),\
> >>  		    sizeof(*(p)))
> > 
> > I like the "count" change -- "n" can be seen as either count or bytes,
> > so I like this being distinctly "count".
> > 
> 
> Yep. :)
> 
> >>  
> >> +/**
> >> + * flex_array_size() - Calculate size, in bytes, of a flexible array member
> >> + * within an enclosing structure. Read on for more details.
> >> + *
> >> + * @p: Pointer to the structure.
> >> + * @member: Name of the flexible array member.
> >> + * @count: Number of elements in the array.
> >> + *
> >> + * Calculates size, in bytes, of a flexible array @member of @count elements
> >> + * within structure @p.
> >> + *
> >> + * Return: number of bytes needed or SIZE_MAX on overflow.
> >> + */
> >> +#define flex_array_size(p, member, count)					\
> >> +	array_size(count,							\
> >> +		    sizeof(*(p)->member) + __must_be_array((p)->member))
> >> +
> >>  #endif /* __LINUX_OVERFLOW_H */
> > 
> > I like it! You mentioned off-list that maybe this could be named
> > sizeof_flex_array() (like sizeof_field(), etc), and that does seem
> > attractive. As you also mentioned, it begs the question of renaming
> > struct_size() to sizeof_struct().
> > 
> > Looking back through the thread[1], it seems the name came from Linus[2],
> > and was more related to the existing array_size() helper.
> > 
> > So, how about this, as a convention we can use to make a choice:
> > 
> > For things that are strictly constant in size, we can use sizeof_*. For
> > things that have a dynamic component, we'll use *_size(). So, this patch
> > is correct as-is.
> > 
> 
> I like the idea. I haven't thought it in terms of dynamic and constant size,
> but it sounds sensible.
> 
> > Acked-by: Kees Cook <keescook@chromium.org>
> > 
> > (I wonder who's tree this should go via?)
> > 
> 
> Yours? :)

Done; I'll see if Linus will take this for -rc2 so you'll be able to use
it for v5.9 patches...

Thanks!

-- 
Kees Cook