Re: [Intel-wired-lan] [PATCH iwl-next] ice: Consistently use ethtool_puts() to copy strings

[Simon Horman <horms@kernel.org>]

On Mon, Sep 02, 2024 at 01:55:41PM -0600, Gustavo A. R. Silva wrote:
> 
> 
> On 02/09/24 06:46, Simon Horman wrote:
> > ethtool_puts() is the preferred method for copying ethtool strings.
> > And ethtool_puts() is already used to copy ethtool strings in
> > igc_ethtool_get_strings(). With this patch igc_ethtool_get_strings()
> > uses it for all such cases.
> > 
> > In general, the compiler can't use fortification to verify that the
> > destination buffer isn't over-run when the destination is the first
> > element of an array, and more than one element of the array is to be
> > written by memcpy().
> > 
> > For the ETH_SS_PRIV_FLAGS the problem doesn't manifest as there is only
> > one element in the igc_priv_flags_strings array.
> > 
> > In the ETH_SS_TEST case, there is more than one element of
> > igc_gstrings_test, and from the compiler's perspective, that element is
> > overrun. In practice it does not overrun the overall size of the array,
> > but it is nice to use tooling to help us where possible. In this case
> > the problem is flagged as follows.
> > 
> > Flagged by clang-18 as:
> > 
> > In file included from drivers/net/ethernet/intel/igc/igc_ethtool.c:5:
> > In file included from ./include/linux/if_vlan.h:10:
> > In file included from ./include/linux/netdevice.h:24:
> > In file included from ./include/linux/timer.h:6:
> > In file included from ./include/linux/ktime.h:25:
> > In file included from ./include/linux/jiffies.h:10:
> > In file included from ./include/linux/time.h:60:
> > In file included from ./include/linux/time32.h:13:
> > In file included from ./include/linux/timex.h:67:
> > In file included from ./arch/x86/include/asm/timex.h:5:
> > In file included from ./arch/x86/include/asm/processor.h:19:
> > In file included from ./arch/x86/include/asm/cpuid.h:62:
> > In file included from ./arch/x86/include/asm/paravirt.h:21:
> > In file included from ./include/linux/cpumask.h:12:
> > In file included from ./include/linux/bitmap.h:13:
> > In file included from ./include/linux/string.h:374:
> > .../fortify-string.h:580:4: warning: call to '__read_overflow2_field' declared with 'warning' attribute: detected read beyond size of field (2nd parameter); maybe use struct_group()? [-Wattribute-warning]
> > 
> > And Smatch as:
> > 
> > .../igc_ethtool.c:771 igc_ethtool_get_strings() error: __builtin_memcpy() '*igc_gstrings_test' too small (32 vs 160)
> > 
> > Curiously, not flagged by gcc-14.
> > 
> > Compile tested only.
> > 
> > Signed-off-by: Simon Horman <horms@kernel.org>
> > ---
> >   drivers/net/ethernet/intel/igc/igc_ethtool.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > index 457b5d7f1610..ccace77c6c2d 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > @@ -768,8 +768,8 @@ static void igc_ethtool_get_strings(struct net_device *netdev, u32 stringset,
> >   	switch (stringset) {
> >   	case ETH_SS_TEST:
> > -		memcpy(data, *igc_gstrings_test,
> > -		       IGC_TEST_LEN * ETH_GSTRING_LEN);
> 
> I think this problem should be solved if we use the array's address,
> which in this case is `igc_gstrings_test`, instead of the address of
> the first row. So, the above should look as follows:
> 
> memcpy(data, igc_gstrings_test, IGC_TEST_LEN * ETH_GSTRING_LEN);

Thanks for the advice.
FWIIW, I do like the consistency of using ethtool_puts().
But, OTOH, your suggestion is much simpler.
I will send an updated the patch accordingly.

> 
> > +		for (i = 0; i < IGC_TEST_LEN; i++)
> > +			ethtool_puts(&p, igc_gstrings_test[i]);
> >   		break;
> >   	case ETH_SS_STATS:
> >   		for (i = 0; i < IGC_GLOBAL_STATS_LEN; i++)
> > @@ -791,8 +791,8 @@ static void igc_ethtool_get_strings(struct net_device *netdev, u32 stringset,
> >   		/* BUG_ON(p - data != IGC_STATS_LEN * ETH_GSTRING_LEN); */
> >   		break;
> >   	case ETH_SS_PRIV_FLAGS:
> > -		memcpy(data, igc_priv_flags_strings,
> > -		       IGC_PRIV_FLAGS_STR_LEN * ETH_GSTRING_LEN);
> 
> In this case, the code is effectively reading from the array's address.

True. In light of your other suggestion I'll drop this hung.