On Tue, Apr 15, 2025 at 6:02 PM Kees Cook <[email protected]> wrote: > > Many drivers populate the stats buffer using C-String based APIs (e.g. > ethtool_sprintf() and ethtool_puts()), usually when building up the > list of stats individually (i.e. with a for() loop). This, however, > requires that the source strings be populated in such a way as to have > a terminating NUL byte in the source. > > Other drivers populate the stats buffer directly using one big memcpy() > of an entire array of strings. No NUL termination is needed here, as the > bytes are being directly passed through. Yet others will build up the > stats buffer individually, but also use memcpy(). This, too, does not > need NUL termination of the source strings. > > However, there are cases where the strings that populate the > source stats strings are exactly ETH_GSTRING_LEN long, and GCC > 15's -Wunterminated-string-initialization option complains that the > trailing NUL byte has been truncated. This situation is fine only if the > driver is using the memcpy() approach. If the C-String APIs are used, > the destination string name will have its final byte truncated by the > required trailing NUL byte applied by the C-string API. > > For drivers that are already using memcpy() but have initializers that > truncate the NUL terminator, mark their source strings as __nonstring to > silence the GCC warnings. > > For drivers that have initializers that truncate the NUL terminator and > are using the C-String APIs, switch to memcpy() to avoid destination > string truncation and mark their source strings as __nonstring to silence > the GCC warnings. (Also introduce ethtool_cpy() as a helper to make this > an easy replacement). > > Specifically the following warnings were investigated and addressed: > > ../drivers/net/ethernet/chelsio/cxgb/cxgb2.c:364:9: warning: > initializer-string for array of 'char' truncates NUL terminator but > destination lacks 'nonstring' attribute (33 chars into 32 available) > [-Wunterminated-string-initialization] > 364 | "TxFramesAbortedDueToXSCollisions", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:165:33: warning: > initializer-string for array of 'char' truncates NUL terminator but > destination lacks 'nonstring' attribute (33 chars into 32 available) > [-Wunterminated-string-initialization] > 165 | { ENETC_PM_R1523X(0), "MAC rx 1523 to max-octet packets" }, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../drivers/net/ethernet/freescale/enetc/enetc_ethtool.c:190:33: warning: > initializer-string for array of 'char' truncates NUL terminator but > destination lacks 'nonstring' attribute (33 chars into 32 available) > [-Wunterminated-string-initialization] > 190 | { ENETC_PM_T1523X(0), "MAC tx 1523 to max-octet packets" }, > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../drivers/net/ethernet/google/gve/gve_ethtool.c:76:9: warning: > initializer-string for array of 'char' truncates NUL terminator but > destination lacks 'nonstring' attribute (33 chars into 32 available) > [-Wunterminated-string-initialization] > 76 | "adminq_dcfg_device_resources_cnt", > "adminq_set_driver_parameter_cnt", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:117:53: warning: > initializer-string for array of 'char' truncates NUL terminator but > destination lacks 'nonstring' attribute (33 chars into 32 available) > [-Wunterminated-string-initialization] > 117 | STMMAC_STAT(ptp_rx_msg_type_pdelay_follow_up), > | ^ > ../drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c:46:12: note: in > definition of macro 'STMMAC_STAT' > 46 | { #m, sizeof_field(struct stmmac_extra_stats, m), \ > | ^ > ../drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c:328:24: warning: > initializer-string for array of 'char' truncates NUL terminator but > destination lacks 'nonstring' attribute (33 chars into 32 available) > [-Wunterminated-string-initialization] > 328 | .str = "a_mac_control_frames_transmitted", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c:340:24: warning: > initializer-string for array of 'char' truncates NUL terminator but > destination lacks 'nonstring' attribute (33 chars into 32 available) > [-Wunterminated-string-initialization] > 340 | .str = "a_pause_mac_ctrl_frames_received", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Signed-off-by: Kees Cook <[email protected]>
Thanks for the patch! For the gve part: Reviewed-by: Harshitha Ramamurthy <[email protected]> > --- > Cc: Jakub Kicinski <[email protected]> > Cc: Andrew Lunn <[email protected]> > Cc: "David S. Miller" <[email protected]> > Cc: Eric Dumazet <[email protected]> > Cc: Paolo Abeni <[email protected]> > Cc: Claudiu Manoil <[email protected]> > Cc: Vladimir Oltean <[email protected]> > Cc: Wei Fang <[email protected]> > Cc: Clark Wang <[email protected]> > Cc: Jeroen de Borst <[email protected]> > Cc: Harshitha Ramamurthy <[email protected]> > Cc: Ido Schimmel <[email protected]> > Cc: Petr Machata <[email protected]> > Cc: Maxime Coquelin <[email protected]> > Cc: Alexandre Torgue <[email protected]> > Cc: Simon Horman <[email protected]> > Cc: Geoff Levand <[email protected]> > Cc: Wolfram Sang <[email protected]> > Cc: Alexander Lobakin <[email protected]> > Cc: Praveen Kaligineedi <[email protected]> > Cc: Willem de Bruijn <[email protected]> > Cc: Joshua Washington <[email protected]> > Cc: Furong Xu <[email protected]> > Cc: "Russell King (Oracle)" <[email protected]> > Cc: Jisheng Zhang <[email protected]> > Cc: Petr Tesarik <[email protected]> > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > Cc: [email protected] > --- > drivers/net/ethernet/chelsio/cxgb/cxgb2.c | 2 +- > drivers/net/ethernet/freescale/enetc/enetc_ethtool.c | 4 ++-- > drivers/net/ethernet/google/gve/gve_ethtool.c | 4 ++-- > .../net/ethernet/mellanox/mlxsw/spectrum_ethtool.c | 2 +- > drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 +- > include/linux/ethtool.h | 11 +++++++++++ > 6 files changed, 18 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/chelsio/cxgb/cxgb2.c > b/drivers/net/ethernet/chelsio/cxgb/cxgb2.c > index 3b7068832f95..4a0e2d2eb60a 100644 > --- a/drivers/net/ethernet/chelsio/cxgb/cxgb2.c > +++ b/drivers/net/ethernet/chelsio/cxgb/cxgb2.c > @@ -351,7 +351,7 @@ static void set_msglevel(struct net_device *dev, u32 val) > adapter->msg_enable = val; > } > > -static const char stats_strings[][ETH_GSTRING_LEN] = { > +static const char stats_strings[][ETH_GSTRING_LEN] __nonstring_array = { > "TxOctetsOK", > "TxOctetsBad", > "TxUnicastFramesOK", > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > index ece3ae28ba82..f47c8b6cc511 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c > @@ -141,7 +141,7 @@ static const struct { > > static const struct { > int reg; > - char name[ETH_GSTRING_LEN]; > + char name[ETH_GSTRING_LEN] __nonstring; > } enetc_port_counters[] = { > { ENETC_PM_REOCT(0), "MAC rx ethernet octets" }, > { ENETC_PM_RALN(0), "MAC rx alignment errors" }, > @@ -264,7 +264,7 @@ static void enetc_get_strings(struct net_device *ndev, > u32 stringset, u8 *data) > break; > > for (i = 0; i < ARRAY_SIZE(enetc_port_counters); i++) > - ethtool_puts(&data, enetc_port_counters[i].name); > + ethtool_cpy(&data, enetc_port_counters[i].name); > > break; > } > diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c > b/drivers/net/ethernet/google/gve/gve_ethtool.c > index eae1a7595a69..3c1da0cf3f61 100644 > --- a/drivers/net/ethernet/google/gve/gve_ethtool.c > +++ b/drivers/net/ethernet/google/gve/gve_ethtool.c > @@ -67,7 +67,7 @@ static const char gve_gstrings_tx_stats[][ETH_GSTRING_LEN] > = { > "tx_xsk_sent[%u]", "tx_xdp_xmit[%u]", "tx_xdp_xmit_errors[%u]" > }; > > -static const char gve_gstrings_adminq_stats[][ETH_GSTRING_LEN] = { > +static const char gve_gstrings_adminq_stats[][ETH_GSTRING_LEN] > __nonstring_array = { > "adminq_prod_cnt", "adminq_cmd_fail", "adminq_timeouts", > "adminq_describe_device_cnt", "adminq_cfg_device_resources_cnt", > "adminq_register_page_list_cnt", "adminq_unregister_page_list_cnt", > @@ -113,7 +113,7 @@ static void gve_get_strings(struct net_device *netdev, > u32 stringset, u8 *data) > i); > > for (i = 0; i < ARRAY_SIZE(gve_gstrings_adminq_stats); i++) > - ethtool_puts(&s, gve_gstrings_adminq_stats[i]); > + ethtool_cpy(&s, gve_gstrings_adminq_stats[i]); > > break; > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c > b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c > index 3f64cdbabfa3..0a8fb9c842d3 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ethtool.c > @@ -262,7 +262,7 @@ static int mlxsw_sp_port_set_pauseparam(struct net_device > *dev, > } > > struct mlxsw_sp_port_hw_stats { > - char str[ETH_GSTRING_LEN]; > + char str[ETH_GSTRING_LEN] __nonstring; > u64 (*getter)(const char *payload); > bool cells_bytes; > }; > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > index 918a32f8fda8..844f7d516a40 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > @@ -37,7 +37,7 @@ > #define ETHTOOL_DMA_OFFSET 55 > > struct stmmac_stats { > - char stat_string[ETH_GSTRING_LEN]; > + char stat_string[ETH_GSTRING_LEN] __nonstring; > int sizeof_stat; > int stat_offset; > }; > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index 013d25858642..7edb5f5e7134 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -1330,6 +1330,17 @@ extern __printf(2, 3) void ethtool_sprintf(u8 **data, > const char *fmt, ...); > */ > extern void ethtool_puts(u8 **data, const char *str); > > +/** > + * ethtool_cpy - Write possibly-not-NUL-terminated string to ethtool string > data > + * @data: Pointer to a pointer to the start of string to write into > + * @str: NUL-byte padded char array of size ETH_GSTRING_LEN to copy from > + */ > +#define ethtool_cpy(data, str) do { \ > + BUILD_BUG_ON(sizeof(str) != ETH_GSTRING_LEN); \ > + memcpy(*(data), str, ETH_GSTRING_LEN); \ > + *(data) += ETH_GSTRING_LEN; \ > +} while (0) > + > /* Link mode to forced speed capabilities maps */ > struct ethtool_forced_speed_map { > u32 speed; > -- > 2.34.1 >
