From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from omta38.uswest2.a.cloudfilter.net (omta38.uswest2.a.cloudfilter.net [35.89.44.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 69E1B1A3A84 for ; Mon, 2 Sep 2024 19:55:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=35.89.44.37 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1725306953; cv=none; b=qGqWLiXznzE82ZOI+lXJm4Igzsk0sOtWVm/5JjwQQHKRqSqRIwhKGrPylA2BYEHG1bdlLGq06L+4sVwY6Q//EIF11hYkkP0wwng1fVVs2S+RAj9RHw0x59VcPFAUYUBSPOn7oxVWPCeqv/VPOHNyCoHN+HfvujqLQ1QJ9brdTbU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1725306953; c=relaxed/simple; bh=4Gl0HPrKQMl+WMydtbAQMHf8HGp50fdjNpDI/SdAZuE=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=lAHcf4JxTvJ1Oh+RtkFWFNj0o/GArkD3TW2j3s90zUMsTYydhYcUSzZpUHYunRxskvN0010+lrQBg3/pHY05gdo39KA8954HbgSD5MefXYipFJHv9xuhUgwH/uMeL18WENXqmi5P+h8iCgQt064Z7bUVrhofQCRpDmq2y7XRRfo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=embeddedor.com; spf=pass smtp.mailfrom=embeddedor.com; dkim=pass (2048-bit key) header.d=embeddedor.com header.i=@embeddedor.com header.b=bRny1xjM; arc=none smtp.client-ip=35.89.44.37 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=embeddedor.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=embeddedor.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=embeddedor.com header.i=@embeddedor.com header.b="bRny1xjM" Received: from eig-obgw-6003a.ext.cloudfilter.net ([10.0.30.151]) by cmsmtp with ESMTPS id lBltsWlDUumtXlD9NsDYVC; Mon, 02 Sep 2024 19:55:45 +0000 Received: from gator4166.hostgator.com ([108.167.133.22]) by cmsmtp with ESMTPS id lD9MshUk5V2ivlD9MsLV2T; Mon, 02 Sep 2024 19:55:44 +0000 X-Authority-Analysis: v=2.4 cv=OLns3jaB c=1 sm=1 tr=0 ts=66d61840 a=1YbLdUo/zbTtOZ3uB5T3HA==:117 a=frY+GlAHrI6frpeK1MvySw==:17 a=IkcTkHD0fZMA:10 a=EaEq8P2WXUwA:10 a=7T7KSl7uo7wA:10 a=VwQbUJbxAAAA:8 a=62GdtKjr0Ia7E08erwkA:9 a=QEXdDO2ut3YA:10 a=Xt_RvD8W3m28Mn_h3AK8:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=embeddedor.com; s=default; h=Content-Transfer-Encoding:Content-Type: In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender :Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help: List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=GhQ01iJSLK8ItBVKt2WxUw/tblsKhoLVEspih28gtqc=; b=bRny1xjMpunawdNKG0yX95lgsl jOo3JpFSGkfAHDsF7pyEkh++AQkoIVnB7whs6Cgr+9fQkull+32tyqq+3e2v7k/ro7YkdiPhKPGUO QAsPpkA0vBW5aFA8uPjx/kgQuluZ8jN999AiCS4htzyW2f71Ha188kwLBgpr3AoOwPazJNdNj6gfP LqxneShjYOBCcqPPvieO8yOJaNdO6HlhwgDTrk5ZysrDonCs7STjOVSnNjvtWjDSSa4BHHIc0+bZx NFpe+O3TRRgQFYNwSvs6xen8gzarK2H24b8v+xem8E+Th/4vnogFqOeHyubchzOiqv5CTzOmNh4Ok FpbioynA==; Received: from [201.172.173.139] (port=56798 helo=[192.168.15.5]) by gator4166.hostgator.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96.2) (envelope-from ) id 1slD9L-0048xZ-0r; Mon, 02 Sep 2024 14:55:43 -0500 Message-ID: <88384607-4fcf-4ab1-8edf-9258df0bbf3c@embeddedor.com> Date: Mon, 2 Sep 2024 13:55:41 -0600 Precedence: bulk X-Mailing-List: llvm@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [Intel-wired-lan] [PATCH iwl-next] ice: Consistently use ethtool_puts() to copy strings To: Simon Horman , Tony Nguyen , Przemek Kitszel Cc: netdev@vger.kernel.org, llvm@lists.linux.dev, Nick Desaulniers , Nathan Chancellor , Eric Dumazet , intel-wired-lan@lists.osuosl.org, Bill Wendling , Justin Stitt , Jakub Kicinski , Paolo Abeni , "David S. Miller" References: <20240902-igc-ss-puts-v1-1-c66a73b532c7@kernel.org> Content-Language: en-US From: "Gustavo A. R. Silva" In-Reply-To: <20240902-igc-ss-puts-v1-1-c66a73b532c7@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - gator4166.hostgator.com X-AntiAbuse: Original Domain - lists.linux.dev X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - embeddedor.com X-BWhitelist: no X-Source-IP: 201.172.173.139 X-Source-L: No X-Exim-ID: 1slD9L-0048xZ-0r X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: ([192.168.15.5]) [201.172.173.139]:56798 X-Source-Auth: gustavo@embeddedor.com X-Email-Count: 11 X-Org: HG=hgshared;ORG=hostgator; X-Source-Cap: Z3V6aWRpbmU7Z3V6aWRpbmU7Z2F0b3I0MTY2Lmhvc3RnYXRvci5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfPzI1JtD9Lu4TI4ROiTMJb6LfLxRJhvXsz7VZrSfX1oonACzY9eLODpoenBFsTLiY642hW9dHhO6My+4Fe6q8ds+DUb0rqugyaESWaXywr0Wb1zt7kKc d8/oYMdbZ52tJIdc/ZSdr4n8vJ1Qo/6Tw5HIY/A1bAqe/M1OvJTj90VQcZiPCh4dzmpzy3BRsY6idrTYb7oxgakxECvJkvS2NW0= 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 > --- > 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