v2:
- rename flex array helper
- split flex array helper arguments to be more DECLARE_*-like
- added knowledge of flex array helper to kernel-doc
- added reviews
v1: https://lore.kernel.org/lkml/[email protected]/
Hi,
In support of the improved buffer overflow detection for memcpy(),
this enables -Warray-bounds and -Wzero-length-bounds globally. Mostly
it involves some struct member tricks with the new DECLARE_FLEX_ARRAY()
macro. Everything else is just replacing stacked 0-element arrays
with actual unions in two related treewide patches. There is one set of
special cases that were fixed separately[1] and are needed as well.
I'm expecting to carry this series with the memcpy() series in my
"overflow" tree. Reviews appreciated! :)
Thanks!
-Kees
[1] https://lore.kernel.org/lkml/[email protected]/
Kees Cook (5):
stddef: Add flexible array union helper
treewide: Replace open-coded flex arrays in unions
treewide: Replace 0-element memcpy() destinations with flexible arrays
Makefile: Enable -Warray-bounds
Makefile: Enable -Wzero-length-bounds
Makefile | 2 --
drivers/crypto/chelsio/chcr_crypto.h | 14 +++++----
drivers/net/can/usb/etas_es58x/es581_4.h | 2 +-
drivers/net/can/usb/etas_es58x/es58x_fd.h | 2 +-
drivers/net/wireless/ath/ath10k/bmi.h | 10 +++----
drivers/net/wireless/ath/ath10k/htt.h | 7 +++--
.../net/wireless/intel/iwlegacy/commands.h | 6 ++--
.../net/wireless/intel/iwlwifi/dvm/commands.h | 6 ++--
.../net/wireless/intel/iwlwifi/fw/api/tx.h | 6 ++--
drivers/scsi/aic94xx/aic94xx_sds.c | 6 ++--
drivers/scsi/qla4xxx/ql4_def.h | 4 +--
drivers/staging/rtl8188eu/include/ieee80211.h | 6 ++--
drivers/staging/rtl8712/ieee80211.h | 4 +--
drivers/staging/rtl8723bs/include/ieee80211.h | 6 ++--
fs/hpfs/hpfs.h | 8 ++---
include/linux/filter.h | 6 ++--
include/linux/ieee80211.h | 30 +++++++++----------
include/linux/stddef.h | 13 ++++++++
include/scsi/sas.h | 12 +++++---
include/uapi/linux/dlm_device.h | 4 +--
include/uapi/linux/stddef.h | 16 ++++++++++
include/uapi/rdma/rdma_user_rxe.h | 4 +--
include/uapi/sound/asoc.h | 4 +--
scripts/kernel-doc | 2 ++
24 files changed, 115 insertions(+), 65 deletions(-)
--
2.30.2
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[];
};
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. It
is entirely possible to have a flexible array in 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 <[email protected]>
Cc: "Gustavo A. R. Silva" <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
The 0-element arrays that are used as memcpy() destinations are actually
flexible arrays. Adjust their structures accordingly so that memcpy()
can better reason able their destination size (i.e. they need to be seen
as "unknown" length rather than "zero").
In some cases, use of the flex_array() helper is needed when a flexible
array is part of a union.
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Nilesh Javali <[email protected]>
Cc: Manish Rangankar <[email protected]>
Cc: [email protected]
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: Larry Finger <[email protected]>
Cc: Phillip Potter <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Florian Schilhabel <[email protected]>
Cc: Johannes Berg <[email protected]>
Cc: Christophe JAILLET <[email protected]>
Cc: Fabio Aiuto <[email protected]>
Cc: Ross Schmidt <[email protected]>
Cc: Marco Cesati <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/net/wireless/ath/ath10k/bmi.h | 10 +++----
drivers/scsi/qla4xxx/ql4_def.h | 4 +--
drivers/staging/rtl8188eu/include/ieee80211.h | 6 ++--
drivers/staging/rtl8712/ieee80211.h | 4 +--
drivers/staging/rtl8723bs/include/ieee80211.h | 6 ++--
include/linux/ieee80211.h | 30 +++++++++----------
include/uapi/linux/dlm_device.h | 4 +--
7 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/bmi.h b/drivers/net/wireless/ath/ath10k/bmi.h
index f6fadcbdd86e..0685c0d2d4ea 100644
--- a/drivers/net/wireless/ath/ath10k/bmi.h
+++ b/drivers/net/wireless/ath/ath10k/bmi.h
@@ -109,7 +109,7 @@ struct bmi_cmd {
struct {
__le32 addr;
__le32 len;
- u8 payload[0];
+ u8 payload[];
} write_mem;
struct {
__le32 addr;
@@ -138,18 +138,18 @@ struct bmi_cmd {
} rompatch_uninstall;
struct {
__le32 count;
- __le32 patch_ids[0]; /* length of @count */
+ __le32 patch_ids[]; /* length of @count */
} rompatch_activate;
struct {
__le32 count;
- __le32 patch_ids[0]; /* length of @count */
+ __le32 patch_ids[]; /* length of @count */
} rompatch_deactivate;
struct {
__le32 addr;
} lz_start;
struct {
__le32 len; /* max BMI_MAX_DATA_SIZE */
- u8 payload[0]; /* length of @len */
+ u8 payload[]; /* length of @len */
} lz_data;
struct {
u8 name[BMI_NVRAM_SEG_NAME_SZ];
@@ -160,7 +160,7 @@ struct bmi_cmd {
union bmi_resp {
struct {
- u8 payload[0];
+ DECLARE_FLEX_ARRAY(u8, payload);
} read_mem;
struct {
__le32 result;
diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h
index 031569c496e5..69a590546bf9 100644
--- a/drivers/scsi/qla4xxx/ql4_def.h
+++ b/drivers/scsi/qla4xxx/ql4_def.h
@@ -366,13 +366,13 @@ struct qla4_work_evt {
struct {
enum iscsi_host_event_code code;
uint32_t data_size;
- uint8_t data[0];
+ uint8_t data[];
} aen;
struct {
uint32_t status;
uint32_t pid;
uint32_t data_size;
- uint8_t data[0];
+ uint8_t data[];
} ping;
} u;
};
diff --git a/drivers/staging/rtl8188eu/include/ieee80211.h b/drivers/staging/rtl8188eu/include/ieee80211.h
index da6245a77d5d..aa5c1a513495 100644
--- a/drivers/staging/rtl8188eu/include/ieee80211.h
+++ b/drivers/staging/rtl8188eu/include/ieee80211.h
@@ -199,7 +199,7 @@ struct ieee_param {
struct {
u32 len;
u8 reserved[32];
- u8 data[0];
+ u8 data[];
} wpa_ie;
struct {
int command;
@@ -212,7 +212,7 @@ struct ieee_param {
u8 idx;
u8 seq[8]; /* sequence counter (set: RX, get: TX) */
u16 key_len;
- u8 key[0];
+ u8 key[];
} crypt;
#ifdef CONFIG_88EU_AP_MODE
struct {
@@ -224,7 +224,7 @@ struct ieee_param {
} add_sta;
struct {
u8 reserved[2];/* for set max_num_sta */
- u8 buf[0];
+ u8 buf[];
} bcn_ie;
#endif
diff --git a/drivers/staging/rtl8712/ieee80211.h b/drivers/staging/rtl8712/ieee80211.h
index 61eff7c5746b..65ceaca9b51e 100644
--- a/drivers/staging/rtl8712/ieee80211.h
+++ b/drivers/staging/rtl8712/ieee80211.h
@@ -78,7 +78,7 @@ struct ieee_param {
struct {
u32 len;
u8 reserved[32];
- u8 data[0];
+ u8 data[];
} wpa_ie;
struct {
int command;
@@ -91,7 +91,7 @@ struct ieee_param {
u8 idx;
u8 seq[8]; /* sequence counter (set: RX, get: TX) */
u16 key_len;
- u8 key[0];
+ u8 key[];
} crypt;
} u;
};
diff --git a/drivers/staging/rtl8723bs/include/ieee80211.h b/drivers/staging/rtl8723bs/include/ieee80211.h
index 378c21595e05..89c311cd20a6 100644
--- a/drivers/staging/rtl8723bs/include/ieee80211.h
+++ b/drivers/staging/rtl8723bs/include/ieee80211.h
@@ -180,7 +180,7 @@ struct ieee_param {
struct {
u32 len;
u8 reserved[32];
- u8 data[0];
+ u8 data[];
} wpa_ie;
struct{
int command;
@@ -193,7 +193,7 @@ struct ieee_param {
u8 idx;
u8 seq[8]; /* sequence counter (set: RX, get: TX) */
u16 key_len;
- u8 key[0];
+ u8 key[];
} crypt;
struct {
u16 aid;
@@ -204,7 +204,7 @@ struct ieee_param {
} add_sta;
struct {
u8 reserved[2];/* for set max_num_sta */
- u8 buf[0];
+ u8 buf[];
} bcn_ie;
} u;
};
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index a6730072d13a..445597c03cd1 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -1101,7 +1101,7 @@ struct ieee80211_mgmt {
__le16 auth_transaction;
__le16 status_code;
/* possibly followed by Challenge text */
- u8 variable[0];
+ u8 variable[];
} __packed auth;
struct {
__le16 reason_code;
@@ -1110,26 +1110,26 @@ struct ieee80211_mgmt {
__le16 capab_info;
__le16 listen_interval;
/* followed by SSID and Supported rates */
- u8 variable[0];
+ u8 variable[];
} __packed assoc_req;
struct {
__le16 capab_info;
__le16 status_code;
__le16 aid;
/* followed by Supported rates */
- u8 variable[0];
+ u8 variable[];
} __packed assoc_resp, reassoc_resp;
struct {
__le16 capab_info;
__le16 status_code;
- u8 variable[0];
+ u8 variable[];
} __packed s1g_assoc_resp, s1g_reassoc_resp;
struct {
__le16 capab_info;
__le16 listen_interval;
u8 current_ap[ETH_ALEN];
/* followed by SSID and Supported rates */
- u8 variable[0];
+ u8 variable[];
} __packed reassoc_req;
struct {
__le16 reason_code;
@@ -1140,11 +1140,11 @@ struct ieee80211_mgmt {
__le16 capab_info;
/* followed by some of SSID, Supported rates,
* FH Params, DS Params, CF Params, IBSS Params, TIM */
- u8 variable[0];
+ u8 variable[];
} __packed beacon;
struct {
/* only variable items: SSID, Supported rates */
- u8 variable[0];
+ DECLARE_FLEX_ARRAY(u8, variable);
} __packed probe_req;
struct {
__le64 timestamp;
@@ -1152,7 +1152,7 @@ struct ieee80211_mgmt {
__le16 capab_info;
/* followed by some of SSID, Supported rates,
* FH Params, DS Params, CF Params, IBSS Params */
- u8 variable[0];
+ u8 variable[];
} __packed probe_resp;
struct {
u8 category;
@@ -1161,16 +1161,16 @@ struct ieee80211_mgmt {
u8 action_code;
u8 dialog_token;
u8 status_code;
- u8 variable[0];
+ u8 variable[];
} __packed wme_action;
struct{
u8 action_code;
- u8 variable[0];
+ u8 variable[];
} __packed chan_switch;
struct{
u8 action_code;
struct ieee80211_ext_chansw_ie data;
- u8 variable[0];
+ u8 variable[];
} __packed ext_chan_switch;
struct{
u8 action_code;
@@ -1186,7 +1186,7 @@ struct ieee80211_mgmt {
__le16 timeout;
__le16 start_seq_num;
/* followed by BA Extension */
- u8 variable[0];
+ u8 variable[];
} __packed addba_req;
struct{
u8 action_code;
@@ -1202,11 +1202,11 @@ struct ieee80211_mgmt {
} __packed delba;
struct {
u8 action_code;
- u8 variable[0];
+ u8 variable[];
} __packed self_prot;
struct{
u8 action_code;
- u8 variable[0];
+ u8 variable[];
} __packed mesh_action;
struct {
u8 action;
@@ -1250,7 +1250,7 @@ struct ieee80211_mgmt {
u8 toa[6];
__le16 tod_error;
__le16 toa_error;
- u8 variable[0];
+ u8 variable[];
} __packed ftm;
} u;
} __packed action;
diff --git a/include/uapi/linux/dlm_device.h b/include/uapi/linux/dlm_device.h
index f880d2831160..e83954c69fff 100644
--- a/include/uapi/linux/dlm_device.h
+++ b/include/uapi/linux/dlm_device.h
@@ -45,13 +45,13 @@ struct dlm_lock_params {
void __user *bastaddr;
struct dlm_lksb __user *lksb;
char lvb[DLM_USER_LVB_LEN];
- char name[0];
+ char name[];
};
struct dlm_lspace_params {
__u32 flags;
__u32 minor;
- char name[0];
+ char name[];
};
struct dlm_purge_params {
--
2.30.2
With all known internal zero-length accesses fixed, it is possible to
enable -Wzero-length-bounds globally. Since this is included by default
in -Warray-bounds, we just need to stop disabling it.
Cc: "Gustavo A. R. Silva" <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
Makefile | 1 -
1 file changed, 1 deletion(-)
diff --git a/Makefile b/Makefile
index 8e7e73a642e2..8e732e875e78 100644
--- a/Makefile
+++ b/Makefile
@@ -994,7 +994,6 @@ KBUILD_CFLAGS += -Wno-pointer-sign
KBUILD_CFLAGS += $(call cc-disable-warning, stringop-truncation)
# We'll want to enable this eventually, but it's not going away for 5.7 at least
-KBUILD_CFLAGS += $(call cc-disable-warning, zero-length-bounds)
KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow)
# Another good warning that we'll want to enable eventually
--
2.30.2
With the recent fixes for flexible arrays and expanded FORTIFY_SOURCE
coverage, it is now possible to enable -Warray-bounds. Since both
GCC and Clang include -Warray-bounds in -Wall, we just need to stop
disabling it.
Cc: Arnd Bergmann <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: [email protected]
Co-developed-by: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
Makefile | 1 -
1 file changed, 1 deletion(-)
diff --git a/Makefile b/Makefile
index e4f5895badb5..8e7e73a642e2 100644
--- a/Makefile
+++ b/Makefile
@@ -995,7 +995,6 @@ KBUILD_CFLAGS += $(call cc-disable-warning, stringop-truncation)
# We'll want to enable this eventually, but it's not going away for 5.7 at least
KBUILD_CFLAGS += $(call cc-disable-warning, zero-length-bounds)
-KBUILD_CFLAGS += $(call cc-disable-warning, array-bounds)
KBUILD_CFLAGS += $(call cc-disable-warning, stringop-overflow)
# Another good warning that we'll want to enable eventually
--
2.30.2
Kees Cook <[email protected]> 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 <[email protected]>
> Cc: "Gustavo A. R. Silva" <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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
>
On Fri, Aug 27, 2021 at 06:25:32PM +0900, Vincent Mailhol wrote:
> Kees Cook <[email protected]> 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 <[email protected]>
> > Cc: "Gustavo A. R. Silva" <[email protected]>
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > 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