LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
To: Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org
Cc: Vincent Mailhol <mailhol.vincent@wanadoo.fr>,
	Arnd Bergmann <arnd@arndb.de>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	clang-built-linux@googlegroups.com,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2 1/5] stddef: Add flexible array union helper
Date: Fri, 27 Aug 2021 18:25:32 +0900	[thread overview]
Message-ID: <20210827092532.908506-1-mailhol.vincent@wanadoo.fr> (raw)
In-Reply-To: <20210826050458.1540622-1-keescook@chromium.org>

Kees Cook <keescook@chromium.org> writes:
> Many places in the kernel want to use a flexible array in a union. This
> is especially common when wanting several different typed trailing
> flexible arrays. Since GCC and Clang don't (on the surface) allow this,
> such structs have traditionally used combinations of zero-element arrays
> instead. This is usually in the form:
> 
> struct thing {
> 	...
> 	struct type1 foo[0];
> 	struct type2 bar[];
> };

At first read, I found the description confusing (and even thought
that there was a copy/paste issue). The subject and the first sentence
is about "flexible arrays in a *union*". Then suddenly, the topic
shifts to *structs*.

After reading at the code, it is clear that this work for both:
  - unions with a flexible array.
  - structures with different typed trailing flexible arrays.

The subject and the description could be updated to clarify that this
macro can be used for both unions and structs.

N.B. this comment only applies to the commit message, the kerneldoc
part is clear.

> This causes problems with size checks against such zero-element arrays
> (for example with -Warray-bounds and -Wzero-length-bounds), so they must
> all be converted to "real" flexible arrays, avoiding warnings like this:
> 
> fs/hpfs/anode.c: In function 'hpfs_add_sector_to_btree':
> fs/hpfs/anode.c:209:27: warning: array subscript 0 is outside the bounds of an interior zero-length array 'struct bplus_internal_node[0]' [-Wzero-length-bounds]
>   209 |    anode->btree.u.internal[0].down = cpu_to_le32(a);
>       |    ~~~~~~~~~~~~~~~~~~~~~~~^~~
> In file included from fs/hpfs/hpfs_fn.h:26,
>                  from fs/hpfs/anode.c:10:
> fs/hpfs/hpfs.h:412:32: note: while referencing 'internal'
>   412 |     struct bplus_internal_node internal[0]; /* (internal) 2-word entries giving
>       |                                ^~~~~~~~
> 
> drivers/net/can/usb/etas_es58x/es58x_fd.c: In function 'es58x_fd_tx_can_msg':
> drivers/net/can/usb/etas_es58x/es58x_fd.c:360:35: warning: array subscript 65535 is outside the bounds of an interior zero-length array 'u8[0]' {aka 'unsigned char[]'} [-Wzero-length-bounds]
>   360 |  tx_can_msg = (typeof(tx_can_msg))&es58x_fd_urb_cmd->raw_msg[msg_len];
>       |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from drivers/net/can/usb/etas_es58x/es58x_core.h:22,
>                  from drivers/net/can/usb/etas_es58x/es58x_fd.c:17:
> drivers/net/can/usb/etas_es58x/es58x_fd.h:231:6: note: while referencing 'raw_msg'
>   231 |   u8 raw_msg[0];
>       |      ^~~~~~~
> 
> Introduce DECLARE_FLEX_ARRAY() in support of flexible arrays in unions.

... and structures.

> It is entirely possible to have a flexible array in a union:

It is entirely possible to have one or several flexible arrays in a
structure or a union:

> it just has to
> be in a struct. And since it cannot be alone in a struct, such a struct
> must have at least 1 other named member but that member can be zero sized.
> 
> As with struct_group(), this is needed in UAPI headers as well, so
> implement the core there, with non-UAPI wrapper.
> 
> Additionally update kernel-doc to understand its existence.
> 
> https://github.com/KSPP/linux/issues/137
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/stddef.h      | 13 +++++++++++++
>  include/uapi/linux/stddef.h | 16 ++++++++++++++++
>  scripts/kernel-doc          |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/include/linux/stddef.h b/include/linux/stddef.h
> index 8b103a53b000..ca507bd5f808 100644
> --- a/include/linux/stddef.h
> +++ b/include/linux/stddef.h
> @@ -84,4 +84,17 @@ enum {
>  #define struct_group_tagged(TAG, NAME, MEMBERS...) \
>  	__struct_group(TAG, NAME, /* no attrs */, MEMBERS)
>  
> +/**
> + * DECLARE_FLEX_ARRAY() - Declare a flexible array usable in a union
> + *
> + * @TYPE: The type of each flexible array element
> + * @NAME: The name of the flexible array member
> + *
> + * In order to have a flexible array member in a union or alone in a
> + * struct, it needs to be wrapped in an anonymous struct with at least 1
> + * named member, but that member can be empty.
> + */
> +#define DECLARE_FLEX_ARRAY(TYPE, NAME) \
> +	__DECLARE_FLEX_ARRAY(TYPE, NAME)
> +
>  #endif
> diff --git a/include/uapi/linux/stddef.h b/include/uapi/linux/stddef.h
> index 610204f7c275..3021ea25a284 100644
> --- a/include/uapi/linux/stddef.h
> +++ b/include/uapi/linux/stddef.h
> @@ -25,3 +25,19 @@
>  		struct { MEMBERS } ATTRS; \
>  		struct TAG { MEMBERS } ATTRS NAME; \
>  	}
> +
> +/**
> + * __DECLARE_FLEX_ARRAY() - Declare a flexible array usable in a union
> + *
> + * @TYPE: The type of each flexible array element
> + * @NAME: The name of the flexible array member
> + *
> + * In order to have a flexible array member in a union or alone in a
> + * struct, it needs to be wrapped in an anonymous struct with at least 1
> + * named member, but that member can be empty.
> + */
> +#define __DECLARE_FLEX_ARRAY(TYPE, NAME)	\
> +	struct { \
> +		struct { } __empty_ ## NAME; \
> +		TYPE NAME[]; \
> +	}
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index d9715efbe0b7..65088b512d14 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -1263,6 +1263,8 @@ sub dump_struct($$) {
>  	$members =~ s/DECLARE_KFIFO\s*\($args,\s*$args,\s*$args\)/$2 \*$1/gos;
>  	# replace DECLARE_KFIFO_PTR
>  	$members =~ s/DECLARE_KFIFO_PTR\s*\($args,\s*$args\)/$2 \*$1/gos;
> +	# replace DECLARE_FLEX_ARRAY
> +	$members =~ s/(?:__)?DECLARE_FLEX_ARRAY\s*\($args,\s*$args\)/$1 $2\[\]/gos;
>  	my $declaration = $members;
>  
>  	# Split nested struct/union elements as newer ones
> -- 
> 2.30.2
> 

  parent reply	other threads:[~2021-08-27  9:26 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-26  5:04 [PATCH v2 0/5] Enable -Warray-bounds and -Wzero-length-bounds Kees Cook
2021-08-26  5:04 ` [PATCH v2 1/5] stddef: Add flexible array union helper Kees Cook
2021-08-26  5:04 ` [PATCH v2 2/5] treewide: Replace open-coded flex arrays in unions Kees Cook
2021-08-26  6:24   ` Marc Kleine-Budde
2021-08-27 16:08     ` Kees Cook
2021-08-27 17:08       ` Marc Kleine-Budde
2021-08-27 16:17     ` Kees Cook
2021-08-28  7:31       ` Vincent MAILHOL
2021-08-26  7:36   ` Vincent MAILHOL
2021-08-26 15:39     ` Kees Cook
2021-08-26  5:04 ` [PATCH v2 3/5] treewide: Replace 0-element memcpy() destinations with flexible arrays Kees Cook
2021-08-26  5:24   ` Keith Packard
2021-08-26  5:51     ` Kees Cook
2021-08-26  5:04 ` [PATCH v2 4/5] Makefile: Enable -Warray-bounds Kees Cook
2021-08-26  5:04 ` [PATCH v2 5/5] Makefile: Enable -Wzero-length-bounds Kees Cook
2021-08-27  9:25 ` Vincent Mailhol [this message]
2021-08-27 15:39   ` [PATCH v2 1/5] stddef: Add flexible array union helper Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210827092532.908506-1-mailhol.vincent@wanadoo.fr \
    --to=mailhol.vincent@wanadoo.fr \
    --cc=arnd@arndb.de \
    --cc=clang-built-linux@googlegroups.com \
    --cc=gustavoars@kernel.org \
    --cc=keescook@chromium.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH v2 1/5] stddef: Add flexible array union helper' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).