Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <984bcd4d-627e-7d26-2a50-73607ea0eecd@intel.com>
Date:   Thu, 5 Oct 2023 16:53:14 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy
Content-Language: en-US
To:     Kees Cook <keescook@chromium.org>,
        Justin Stitt <justinstitt@google.com>
CC:     Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "Paolo Abeni" <pabeni@redhat.com>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-hardening@vger.kernel.org>
References: <20231005-strncpy-drivers-net-dsa-lan9303-core-c-v1-1-5a66c538147e@google.com>
 <202310041959.727EB5ED@keescook> <202310042201.7B14CA59@keescook>
From:   Alexander Lobakin <aleksander.lobakin@intel.com>
In-Reply-To: <202310042201.7B14CA59@keescook>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?amF3cGc0bDJxbXRxaGs2eUlLVUl0cGlSbDI2N0hWSHRzU3BWenkxMFR5Wnkx?=
 =?utf-8?B?TXNVRW1Eb2cwZHJGNngyN3J4WDMwbDFZWk9TaVFOVy9Pak5SVkRXMkxJRFJV?=
 =?utf-8?B?dnNDTEg0OWU4V3BRb1pkRnRzcDZLWkJqLzJ0UVFuRlVscHlhamw5RG1JczZE?=
 =?utf-8?B?RXNOMGhQTmdNRC9ORENDQ0R4T2k2N0VqS3dBU0JYQzduS2FuckJnMFBZS2s5?=
 =?utf-8?B?TGNiYy9UeGdnWlMyZDVvUVdEVWZML1ltcjlUNmdVL1ZndGMzbzFpL1lqcVNS?=
 =?utf-8?B?cGhJVzFjLzUvY1BIdHV5WGNNdDFEbnNpbkJZeXkrbzZZYzN3Zm0xUkN4VUwy?=
 =?utf-8?B?cmk2SXNxTUJoN1FhNkg2bElISkFDTm14ZkEySjY0UGdtUEtvSzkyOExUbFBx?=
 =?utf-8?B?Z1BXcGdZUHE1WDdjOS9raEQ3UnFDd3hOOWFuOW54dlBKTk0zbDNhczBSdGdF?=
 =?utf-8?B?d1J1eVZLU0xJb3pCbmJydDN6TUFVK2hhWkVEVktiZm9wcWQrOWhOUnVzT1ZE?=
 =?utf-8?B?UkV4WTZBRU9SVHhEL0FlYndOMmZxYXl1ME9BWExlRXIwN2x4T3NCbVlqUmhZ?=
 =?utf-8?B?V01NUzJMRTh1M09LNk5aL0dnSGJJbUhpT2dnZ2pkM3pwdCsxSGRFSjVaWU1y?=
 =?utf-8?B?SzBoQ2ptY3JLendMbEJhQTduVEVPUWJBd1dOZVN1U1dKWkRJQVpCMVpSQjFF?=
 =?utf-8?B?eWUvWGdnSmNVTGRXZStYMmplMjY1a2pIaHBWN0IrSE9RMG4wUEN5LzVwVjhq?=
 =?utf-8?B?RWhGWjh5OVNVaTlNN0xXV1BOdm5na0FNaHpETVR2a01MY1R5VlphRWZsWXlB?=
 =?utf-8?B?dWJodGdJeElDbzRFNit5QXQ0djhQSGNSdk5iRmxXalFEZ3VBNWJCS1VWMXhS?=
 =?utf-8?B?b1hoVndSN3JZMlQ5VWR5SGdMMWhiYWNhenJBS0FSdThDeWQzQWNKcmhadU8y?=
 =?utf-8?B?UFpYR1p3TkNYcmUrK2dBeDVZc2tJUEdmc0JGMjMrOHorYzJ3ZXREOVJ2M1N0?=
 =?utf-8?B?T0crejBoU1ZYdWgyOXpldFNBZTJJekdRMEdSVStVcEZwNDBwOVNKd285NWRh?=
 =?utf-8?B?Q1JLMTduaVV1NS81R1hlbzdQRkd4QlB1NzY3aHRpakdBU25hTGNvYlFzQzgw?=
 =?utf-8?B?WHBicUs3cUduWWNZamF5OHVWYmlSNG1udGhGclRlVUFBSjd6dVlJK2NGcEFQ?=
 =?utf-8?B?QXBXTzZXMEQyUStwSWJ6bFRxQm9oN2NyOTdMM3IzT3BuNWg5LzFPQmU0L0tC?=
 =?utf-8?B?ZFdoTmhWTEIzNGtIanhpaUd3d2pid2ozVnlXZ2tBcE1ReXorRkVucnJYY1lB?=
 =?utf-8?B?TXFjOWtPam1ialF4NzBNZVZTU0w0cFJ0RG41dXEyVjBqK1RQdlMveUZaaUlT?=
 =?utf-8?B?TThKb2hUK2FBN093cC9MY0JVNWhSVHJJZWVHYnJyM0p3QVY1UWpjM1NnNjJL?=
 =?utf-8?B?cVhENnFraUhGdUo2VVpoNXdmSk9kTGZTRExjMm02MG15QXJlbkl2UlB4U1Bh?=
 =?utf-8?B?NVZsYjlKV3hFdTZVVkdqTDU3NUdJZS82WHJhdnhIVEIxZk5mSmdsSkdDd3FD?=
 =?utf-8?B?Vi9lMU95dXZQaStSa3JaKzg0ZTluVnNBL2p5aUZWVmtEMnRyWGpna2dpdkRB?=
 =?utf-8?B?dkpGS2FkR285YXYyQWRYTUdzcUhKTW9oZGRwMEN6V1l2MThDZ1hod2Q3ZVlH?=
 =?utf-8?B?b1dqeStqWHBUazFnVkVHVE9Fb3pMcTZiM3hpczlNU0Z2bTJPbThicldpUkRG?=
 =?utf-8?B?OWRjRzdMOGtucXB4c2dYYTd5R0dvTG5mMUR5d1pQRHNBSlI0SGF0R3R1ckZH?=
 =?utf-8?B?ZGxaWWN3UVowcFAvV0N0a251V09pdkYvT2VjU0JvUUg4OU5rdS9YL3ZXYlpt?=
 =?utf-8?B?b1U5UEhpUW5mcEFOdGJIZ2ljUUp3UFpxMi9KSTlXVTBjaUVyMHhJVUliT2tk?=
 =?utf-8?B?NG9zVVEvMlRVc1RrcmdDWk5XZTQzOGo4dVFmckJYNU9lUlh0c3ZRaXFVbm9N?=
 =?utf-8?B?Y3ozdnh3S2JBUEY0MHpOSGhERHQ4OWFBNHh6dU53NTQyNWEzeHZBbzB6SC8z?=
 =?utf-8?B?V3o5UlJWYmJ1LzZVTUgvTW9UcTA3a2NLS2J6R2RVaVFSSGlENFhuWWxGOGVz?=
 =?utf-8?B?TkwzQXg0YkovdTNmYTk3dTgrSHl3aDlDOCtJNFdqdlMxYjQwaFFVYURmS2NQ?=
 =?utf-8?B?Rnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9c86b7ee-0720-4706-7f70-08dbc5b2fdad
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3625.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2023 14:54:35.6097
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: +vhOQxKjSxbHNc5BFpzRLzJ68nBT98vfBdz+ww//1CGJkIp8W8BSIAGaUkR5qgyjQqu7VuWIizr4WOPYkX7C8NcBw5AH0pY7k9Zg1sruQEQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4838
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.1 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 05 Oct 2023 08:40:19 -0700 (PDT)

From: Kees Cook <keescook@chromium.org>
Date: Wed, 4 Oct 2023 22:02:00 -0700

> On Wed, Oct 04, 2023 at 08:07:55PM -0700, Kees Cook wrote:
>> On Thu, Oct 05, 2023 at 12:30:18AM +0000, Justin Stitt wrote:
>>> `strncpy` is deprecated for use on NUL-terminated destination strings
>>> [1] and as such we should prefer more robust and less ambiguous
>>> interfaces.
>>>
>>> Let's opt for memcpy as we are copying strings into slices of length
>>> `ETH_GSTRING_LEN` within the `data` buffer. Other similar get_strings()
>>> implementations [2] [3] use memcpy().
>>>
>>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
>>> Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/opa_vnic/opa_vnic_ethtool.c#L167 [2]
>>> Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c#L137 [3]
>>> Link: https://github.com/KSPP/linux/issues/90
>>> Cc: linux-hardening@vger.kernel.org
>>> Signed-off-by: Justin Stitt <justinstitt@google.com>
>>> ---
>>> Note: build-tested only.
>>> ---
>>>  drivers/net/dsa/lan9303-core.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
>>> index ee67adeb2cdb..665d69384b62 100644
>>> --- a/drivers/net/dsa/lan9303-core.c
>>> +++ b/drivers/net/dsa/lan9303-core.c
>>> @@ -1013,8 +1013,8 @@ static void lan9303_get_strings(struct dsa_switch *ds, int port,
>>>  		return;
>>>  
>>>  	for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) {
>>> -		strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name,
>>> -			ETH_GSTRING_LEN);
>>> +		memcpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name,
>>> +		       ETH_GSTRING_LEN);
>>
>> This won't work because lan9303_mib entries aren't ETH_GSTRING_LEN-long
>> strings; they're string pointers:
>>
>> static const struct lan9303_mib_desc lan9303_mib[] = {
>>         { .offset = LAN9303_MAC_RX_BRDCST_CNT_0, .name = "RxBroad", },
>>
>> So this really does need a strcpy-family function.
>>
>> And, I think the vnic_gstrings_stats and ipoib_gstrings_stats examples
>> are actually buggy -- they're copying junk into userspace...
>>
>> I am reminded of this patch, which correctly uses strscpy_pad():
>> https://lore.kernel.org/lkml/20230718-net-dsa-strncpy-v1-1-e84664747713@google.com/
>>
>> I think you want to do the same here, and use strscpy_pad(). And perhaps
>> send some fixes for the other memcpy() users?
> 
> Meh, I think it's not worth fixing the memcpy() users of this. This
> buggy pattern is very common, it seems:
> 
> $ git grep 'data.*ETH_GSTRING_LEN' | grep memcpy | wc -l
> 47

We have ethtool_sprintf() precisely for the sake of filling the Ethtool
statistics names.

BTW this weird pattern "let's make the array of our stats names of fixed
width (ETH_GSTRING_LEN), so that we could use memcpy() instead of
strcpy()" was pretty common some time ago, no idea why, as it wastes
memory for tons of \0 padding and provokes issues like the one you
noticed here.

> 

Thanks,
Olek