| Message ID | 20241115204308.3821419-3-kees@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 96c677fca54a28fcfea4dbab9c1f2530bd0a08d1 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | UAPI: ethtool: Avoid flex-array in struct ethtool_link_settings | expand |
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!
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 }; /**
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(+)