LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Cc: linux-kernel@vger.kernel.org, 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 08:39:40 -0700	[thread overview]
Message-ID: <202108270838.10695297AD@keescook> (raw)
In-Reply-To: <20210827092532.908506-1-mailhol.vincent@wanadoo.fr>

On Fri, Aug 27, 2021 at 06:25:32PM +0900, Vincent Mailhol wrote:
> 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.

Yeah, I see now how this doesn't read well. I've rewritten this to
describe the problem better. Thanks! I will send a v3.

-Kees

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

-- 
Kees Cook

      reply	other threads:[~2021-08-27 15:40 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 ` [PATCH v2 1/5] stddef: Add flexible array union helper Vincent Mailhol
2021-08-27 15:39   ` Kees Cook [this message]

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=202108270838.10695297AD@keescook \
    --to=keescook@chromium.org \
    --cc=arnd@arndb.de \
    --cc=clang-built-linux@googlegroups.com \
    --cc=gustavoars@kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mailhol.vincent@wanadoo.fr \
    --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).