netdev - [PATCH iwl-next] ice: Consistently use ethtool

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240902-igc-ss-puts-v1-1-c66a73b532c7@kernel.org>
Date: Mon, 02 Sep 2024 13:46:44 +0100
From: Simon Horman <horms@...nel.org>
To: Tony Nguyen <anthony.l.nguyen@...el.com>, 
 Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, Nathan Chancellor <nathan@...nel.org>, 
 Nick Desaulniers <ndesaulniers@...gle.com>, 
 Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, 
 intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org, 
 llvm@...ts.linux.dev
Subject: [PATCH iwl-next] ice: Consistently use ethtool_puts() to copy
 strings

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@...nel.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);
+		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;
 	}
 }