| Message ID | 20240902-igc-ss-puts-v1-1-c66a73b532c7@kernel.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-next] ice: Consistently use ethtool_puts() to copy strings | expand |
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); > + 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. -- Gustavo
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.
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); + 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); + for (i = 0; i < IGC_PRIV_FLAGS_STR_LEN; i++) + ethtool_puts(&p, igc_priv_flags_strings[i]); break; } }
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(-)