public inbox for llvm@lists.linux.dev
 help / color / mirror / Atom feed
From: Simon Horman <horms@kernel.org>
To: Tony Nguyen <anthony.l.nguyen@intel.com>,
	 Przemek Kitszel <przemyslaw.kitszel@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	 Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	 Paolo Abeni <pabeni@redhat.com>,
	Nathan Chancellor <nathan@kernel.org>,
	 Nick Desaulniers <ndesaulniers@google.com>,
	 Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>,
	 intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
	 llvm@lists.linux.dev
Subject: [PATCH iwl-next] ice: Consistently use ethtool_puts() to copy strings
Date: Mon, 02 Sep 2024 13:46:44 +0100	[thread overview]
Message-ID: <20240902-igc-ss-puts-v1-1-c66a73b532c7@kernel.org> (raw)

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


             reply	other threads:[~2024-09-02 12:47 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-09-02 12:46 Simon Horman [this message]
2024-09-02 19:55 ` [Intel-wired-lan] [PATCH iwl-next] ice: Consistently use ethtool_puts() to copy strings Gustavo A. R. Silva
2024-09-03 15:25   ` Simon Horman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240902-igc-ss-puts-v1-1-c66a73b532c7@kernel.org \
    --to=horms@kernel.org \
    --cc=anthony.l.nguyen@intel.com \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=justinstitt@google.com \
    --cc=kuba@kernel.org \
    --cc=llvm@lists.linux.dev \
    --cc=morbo@google.com \
    --cc=nathan@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=przemyslaw.kitszel@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox