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 <[email protected]> > > --- > > 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.
