struct ethtool_link_settings tends to be used as a header for other
structures that have trailing bytes[1], but has a trailing flexible array
itself. Using this overlapped with other structures leads to ambiguous
object sizing in the compiler, so we want to avoid such situations (which
have caused real bugs in the past). Detecting this can be done with
-Wflex-array-member-not-at-end, which will need to be enabled globally.

Using a tagged struct_group() to create a new ethtool_link_settings_hdr
structure isn't possible as it seems we cannot use the tagged variant of
struct_group() due to syntax issues from C++'s perspective (even within
"extern C")[2]. Instead, we can just leave the offending member defined
in UAPI and remove it from the kernel's view of the structure, as Linux
doesn't actually use this member at all. There is also no change in
size since it was already a flexible array that didn't contribute to
size returned by any use of sizeof().

Reported-by: Jakub Kicinski <kuba@kernel.org>
Closes: https://lore.kernel.org/lkml/20241109100213.262a2fa0@kernel.org/ [2]
Link: https://lore.kernel.org/lkml/0bc2809fe2a6c11dd4c8a9a10d9bd65cccdb559b.1730238285.git.gustavoars@kernel.org/ [1]
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: "Kory Maincent (Dent Project)" <kory.maincent@bootlin.com>
---
 include/uapi/linux/ethtool.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index c405ed63acfa..7e1b3820f91f 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -2526,12 +2526,19 @@ struct ethtool_link_settings {
 	__u8	master_slave_state;
 	__u8	rate_matching;
 	__u32	reserved[7];
+#ifndef __KERNEL__
+	/* Linux builds with -Wflex-array-member-not-at-end but does
+	 * not use the "link_mode_masks" member. Leave it defined for
+	 * userspace for now, and when userspace wants to start using
+	 * -Wfamnae, we'll need a new solution.
+	 */
 	__u32	link_mode_masks[];
 	/* layout of link_mode_masks fields:
 	 * __u32 map_supported[link_mode_masks_nwords];
 	 * __u32 map_advertising[link_mode_masks_nwords];
 	 * __u32 map_lp_advertising[link_mode_masks_nwords];
 	 */
+#endif
 };
 
 /**
-- 
2.34.1

On Fri, 15 Nov 2024 12:43:05 -0800 Kees Cook wrote:
> struct ethtool_link_settings tends to be used as a header for other
> structures that have trailing bytes[1], but has a trailing flexible array
> itself. Using this overlapped with other structures leads to ambiguous
> object sizing in the compiler, so we want to avoid such situations (which
> have caused real bugs in the past). Detecting this can be done with
> -Wflex-array-member-not-at-end, which will need to be enabled globally.
> 
> Using a tagged struct_group() to create a new ethtool_link_settings_hdr
> structure isn't possible as it seems we cannot use the tagged variant of
> struct_group() due to syntax issues from C++'s perspective (even within
> "extern C")[2]. Instead, we can just leave the offending member defined
> in UAPI and remove it from the kernel's view of the structure, as Linux
> doesn't actually use this member at all. There is also no change in
> size since it was already a flexible array that didn't contribute to
> size returned by any use of sizeof().

Perfect. I was starting to doubt if user space needs the member,
ethtool CLI doesn't but looks like NetworkManager does. 
So as you say we'll cross that bridge...

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Thanks!