* [PATCH v2] overflow.h: Add flex_array_size() helper
@ 2020-06-09 1:22 Gustavo A. R. Silva
2020-06-09 6:47 ` Joe Perches
2020-06-10 21:38 ` Kees Cook
0 siblings, 2 replies; 6+ messages in thread
From: Gustavo A. R. Silva @ 2020-06-09 1:22 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-kernel, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] overflow.h: Add flex_array_size() helper
2020-06-09 1:22 [PATCH v2] overflow.h: Add flex_array_size() helper Gustavo A. R. Silva
@ 2020-06-09 6:47 ` Joe Perches
2020-06-09 15:42 ` Kees Cook
2020-06-10 21:38 ` Kees Cook
1 sibling, 1 reply; 6+ messages in thread
From: Joe Perches @ 2020-06-09 6:47 UTC (permalink / raw)
To: Gustavo A. R. Silva, Kees Cook; +Cc: linux-kernel, Gustavo A. R. Silva
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 */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] overflow.h: Add flex_array_size() helper
2020-06-09 6:47 ` Joe Perches
@ 2020-06-09 15:42 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2020-06-09 15:42 UTC (permalink / raw)
To: Joe Perches; +Cc: Gustavo A. R. Silva, linux-kernel, Gustavo A. R. Silva
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] overflow.h: Add flex_array_size() helper
2020-06-09 1:22 [PATCH v2] overflow.h: Add flex_array_size() helper Gustavo A. R. Silva
2020-06-09 6:47 ` Joe Perches
@ 2020-06-10 21:38 ` Kees Cook
2020-06-16 20:48 ` Gustavo A. R. Silva
1 sibling, 1 reply; 6+ messages in thread
From: Kees Cook @ 2020-06-10 21:38 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: linux-kernel, Gustavo A. R. Silva
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] overflow.h: Add flex_array_size() helper
2020-06-10 21:38 ` Kees Cook
@ 2020-06-16 20:48 ` Gustavo A. R. Silva
2020-06-17 3:48 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Gustavo A. R. Silva @ 2020-06-16 20:48 UTC (permalink / raw)
To: Kees Cook, Gustavo A. R. Silva; +Cc: linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] overflow.h: Add flex_array_size() helper
2020-06-16 20:48 ` Gustavo A. R. Silva
@ 2020-06-17 3:48 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2020-06-17 3:48 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: Gustavo A. R. Silva, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-17 3:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-09 1:22 [PATCH v2] overflow.h: Add flex_array_size() helper Gustavo A. R. Silva
2020-06-09 6:47 ` Joe Perches
2020-06-09 15:42 ` Kees Cook
2020-06-10 21:38 ` Kees Cook
2020-06-16 20:48 ` Gustavo A. R. Silva
2020-06-17 3:48 ` Kees Cook
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).