RE: [PATCH] net: ethtool: Adjust exactly ETH_GSTRING_LEN-long stats to use memcpy

[Keller, Jacob E]

> -----Original Message-----
> From: Kees Cook <kees@xxxxxxxxxx>
> Sent: Wednesday, April 16, 2025 10:49 AM
> To: Kory Maincent <kory.maincent@xxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>;
> David S. Miller <davem@xxxxxxxxxxxxx>; Dumazet, Eric <edumazet@xxxxxxxxxx>;
> Paolo Abeni <pabeni@xxxxxxxxxx>; Claudiu Manoil <claudiu.manoil@xxxxxxx>;
> Vladimir Oltean <vladimir.oltean@xxxxxxx>; Wei Fang <wei.fang@xxxxxxx>; Clark
> Wang <xiaoning.wang@xxxxxxx>; Jeroen de Borst <jeroendb@xxxxxxxxxx>;
> Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>; Ido Schimmel
> <idosch@xxxxxxxxxx>; Petr Machata <petrm@xxxxxxxxxx>; Maxime Coquelin
> <mcoquelin.stm32@xxxxxxxxx>; Alexandre Torgue
> <alexandre.torgue@xxxxxxxxxxx>; Simon Horman <horms@xxxxxxxxxx>; Geoff
> Levand <geoff@xxxxxxxxxxxxx>; Wolfram Sang <wsa+renesas@sang-
> engineering.com>; Lobakin, Aleksander <aleksander.lobakin@xxxxxxxxx>; Praveen
> Kaligineedi <pkaligineedi@xxxxxxxxxx>; Willem de Bruijn <willemb@xxxxxxxxxx>;
> Joshua Washington <joshwash@xxxxxxxxxx>; Furong Xu <0x1207@xxxxxxxxx>;
> Russell King (Oracle) <rmk+kernel@xxxxxxxxxxxxxxx>; Jisheng Zhang
> <jszhang@xxxxxxxxxx>; Petr Tesarik <petr@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; Richard Cochran <richardcochran@xxxxxxxxx>; Keller,
> Jacob E <jacob.e.keller@xxxxxxxxx>; Shannon Nelson
> <shannon.nelson@xxxxxxx>; Ziwei Xiao <ziweixiao@xxxxxxxxxx>; Shailend
> Chand <shailend@xxxxxxxxxx>; Choong Yong Liang
> <yong.liang.choong@xxxxxxxxxxxxxxx>; Andrew Halaney <ahalaney@xxxxxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx; linux-hardening@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] net: ethtool: Adjust exactly ETH_GSTRING_LEN-long stats to
> use memcpy
> 
> On Wed, Apr 16, 2025 at 11:03:51AM +0200, Kory Maincent wrote:
> > On Tue, 15 Apr 2025 18:02:15 -0700
> > Kees Cook <kees@xxxxxxxxxx> 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.
> >
> > Shouldn't we move on to ethtool_cpy in these drivers too to unify the code?
> 
> I decided that the code churn wasn't worth it. Perhaps in a follow-up
> patch if folks want to see the removal of the explicit memcpy() uses?
> 

A follow-up seems better to me.

Thanks,
Jake

> --
> Kees Cook