netdev - Re: [Intel-wired-lan] [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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <88384607-4fcf-4ab1-8edf-9258df0bbf3c@embeddedor.com>
Date: Mon, 2 Sep 2024 13:55:41 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Simon Horman <horms@...nel.org>, Tony Nguyen
 <anthony.l.nguyen@...el.com>, Przemek Kitszel <przemyslaw.kitszel@...el.com>
Cc: netdev@...r.kernel.org, llvm@...ts.linux.dev,
 Nick Desaulniers <ndesaulniers@...gle.com>,
 Nathan Chancellor <nathan@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 intel-wired-lan@...ts.osuosl.org, Bill Wendling <morbo@...gle.com>,
 Justin Stitt <justinstitt@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ice: Consistently use
 ethtool_puts() to copy strings



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

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