Message ID | 20231012-strncpy-drivers-net-phy-nxp-tja11xx-c-v1-1-5ad6c9dff5c4@google.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2908:b0:403:3b70:6f57 with SMTP id ib8csp1532307vqb; Thu, 12 Oct 2023 15:26:11 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHMHuCN+H/oaCXirWKt5836YxzPjiNKC/eiMtCLN2/00XDoQk2dUupuk9JPOeHZAyLfy+dC X-Received: by 2002:a17:902:d512:b0:1bf:349f:b85c with SMTP id b18-20020a170902d51200b001bf349fb85cmr28045061plg.1.1697149571618; Thu, 12 Oct 2023 15:26:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697149571; cv=none; d=google.com; s=arc-20160816; b=CK43Bg2aCzM/KBOqeNuqQ0NY0fh8sxPeMJw8wRPMVdA8FdNKhe89JFTbVmhHd0trNk T76ylYs2sfknDd09DG9E58RdhA2+98zf0B5L6qcl9umm/gh0GRjyDlUJiWbmqY83Ua0U 7Jx7okXj6vCFnX7AfBgDuxOMEiBNsYoZW+gkW03GzMlvtCIbRj5C1TJgQtLxUGNaQB/s nerfmdOlJ3TEs8a5LuS+o3HHyyI5cXfI7YpZ5SnmuKzndU+P04wPEMSACWRiTuI/Tjnw 9BJN5ML9hrvnNVmbsvGL7JUO02lKwJDUvyC3y2S/0Kfl/wYxo2+6+tCE+kEwwFTSNiME rD7A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:from:subject:message-id:mime-version:date :dkim-signature; bh=4cW7wFWvnFRZJ8ywKO4YSs7fPF7BTc9LFrtaZSrdIDc=; fh=jNFzfs0LVcNPJYpz3MO9rP5uq1TU3TrcBg7mkF9EWrg=; b=ewmWmiofFHD/vGQ4rIjtdkBaytMZdsoDHLZKgcLpY34LJX0aC6cp0BDdglekG5XGcm h8D7LTU+RIa8SCRA/eyePyQSC04ri4vjuqP+CE53jvBbGhFh2Cbkul+N/UZZP37UZjVB f4SCD3+pw3K7m47Fo1U5Xp+7nxExASzqPZWsgGFGOCQIVE2IrhOnHurJ/oZy8sqWzOZO j9aEK+8Y/xdNUaxOkHJn2Kzqr7b6mSIWqkARnOy5CNMcXxi/h8DVjwuVn8+SzDMl1sIE +pLFw16Br/f6ahrHlYZtz0ATIen16nlh/awiKERetSQx0kPjvF/1WP7AiTG3hBERNSmc IRJw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=MBlYxber; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from lipwig.vger.email (lipwig.vger.email. [2620:137:e000::3:3]) by mx.google.com with ESMTPS id x1-20020a1709028ec100b001c61af1e683si2994840plo.641.2023.10.12.15.26.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Oct 2023 15:26:11 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20230601 header.b=MBlYxber; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id D404980B121A; Thu, 12 Oct 2023 15:25:37 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347422AbjJLWZT (ORCPT <rfc822;rua109.linux@gmail.com> + 19 others); Thu, 12 Oct 2023 18:25:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40880 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344200AbjJLWZQ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 12 Oct 2023 18:25:16 -0400 Received: from mail-yw1-x1149.google.com (mail-yw1-x1149.google.com [IPv6:2607:f8b0:4864:20::1149]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B3343BB for <linux-kernel@vger.kernel.org>; Thu, 12 Oct 2023 15:25:15 -0700 (PDT) Received: by mail-yw1-x1149.google.com with SMTP id 00721157ae682-5a7dd655566so23076187b3.2 for <linux-kernel@vger.kernel.org>; Thu, 12 Oct 2023 15:25:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1697149515; x=1697754315; darn=vger.kernel.org; h=cc:to:from:subject:message-id:mime-version:date:from:to:cc:subject :date:message-id:reply-to; bh=4cW7wFWvnFRZJ8ywKO4YSs7fPF7BTc9LFrtaZSrdIDc=; b=MBlYxberzusxJ3c/QS12vHfR2P2xraSd0tr179NcFgrWQUwWpbzoSIWSuQIfyV/qcM +STqKyyBcA1F/Ero3F4X1cqB/dEHUS95F+blljG8Fs5zggeiTkPvLCTGBeiZh9OHCwp6 pJkLpTWGcfdG1fEPXUEar7zTf5YE2mJmRkNjyFh4msqV5pAeRsCXBpoaIxvFZkbTRg5G a29PfL8pMRWPlZdONRX9kJcDnoc2XK/WJQZLEDF04M6ZUN9uky39bVRcXUG03VL/hAe5 3J6eQNG1ndhcoEgqy/AwzNayf+OnB2VnCQcNV/i09IKUo/MnlEz/1iy0ns0NW8ttlBL2 wAUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697149515; x=1697754315; h=cc:to:from:subject:message-id:mime-version:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=4cW7wFWvnFRZJ8ywKO4YSs7fPF7BTc9LFrtaZSrdIDc=; b=cxL8s545nWI/x00yYC9MMOnS0ZJ9+BtZO3MtYfbAOxO2lob6q3BISyModJ1ne4OiRk t8UfZy2jaIaW4APiu3eOCUf0bozowfNqNis0RGI/Y0M5fSaw/I6sQB9cE+EuuP+w0dyq TSrtrjuEnKEBLaco6DiAd4T6OPUWOWgpK2quNk6Xf0Q2jCvyPIlG4qQ7rO6uaPQn40zg L1HSev3uT0OKoy8tIbMZCLfT3taHW9RSG1su7ZG1fO5Kdgj4lMfIpAkP+GXo6CRjei5t Au6qckr5a0uy+nKe9XZcfAuWK6GsXmGW3vpzpAdRvXcCnU1eXtr28F4k7ccSo/es/QzN dh2Q== X-Gm-Message-State: AOJu0YzBxv5vP0QTq/cXGpwwcS5G/OO7tjKIvrhp8A3XqnCScfCHZXEX aS1YNJKJEZKGG29kEpOm68wcc7cFUi+Z5n9Vow== X-Received: from jstitt-linux1.c.googlers.com ([fda3:e722:ac3:cc00:2b:ff92:c0a8:23b5]) (user=justinstitt job=sendgmr) by 2002:a81:d444:0:b0:59b:ebe0:9fcd with SMTP id g4-20020a81d444000000b0059bebe09fcdmr542887ywl.7.1697149515012; Thu, 12 Oct 2023 15:25:15 -0700 (PDT) Date: Thu, 12 Oct 2023 22:25:12 +0000 Mime-Version: 1.0 X-B4-Tracking: v=1; b=H4sIAEdyKGUC/x3NMQ7CMAxA0atUnrFkBwbCVRBDSQw1Q4jsqEpV9 e5EjG/5fwcXU3G4TTuYrOr6LQN8miAtc3kLah6GQOHMxAG9WUl1w2y6ijkWaViXDUuv2D4zc++ YMEbiSFd6cr7AaFWTl/b/5/44jh9NOHRxdwAAAA== X-Developer-Key: i=justinstitt@google.com; a=ed25519; pk=tC3hNkJQTpNX/gLKxTNQKDmiQl6QjBNCGKJINqAdJsE= X-Developer-Signature: v=1; a=ed25519-sha256; t=1697149514; l=1614; i=justinstitt@google.com; s=20230717; h=from:subject:message-id; bh=++HwxaEbHmOYVMN+LAm7g1M8JgRA8xefB8jT1C9ZOnM=; b=ze8sDKCjWG59fZljQnv44JBnL9EeMTQjI7aP0ag/CneGCWxrrlELOiVc5Y4ZRsDDwIQy0qlw1 dd/v50D7pV7Af+Kr5HJ0RA5X6YoyFLaIKVSBeMLSlJjQ3dsgRe3RsFm X-Mailer: b4 0.12.3 Message-ID: <20231012-strncpy-drivers-net-phy-nxp-tja11xx-c-v1-1-5ad6c9dff5c4@google.com> Subject: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf From: Justin Stitt <justinstitt@google.com> To: Andrew Lunn <andrew@lunn.ch>, Heiner Kallweit <hkallweit1@gmail.com>, Russell King <linux@armlinux.org.uk>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com> Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org, Justin Stitt <justinstitt@google.com> Content-Type: text/plain; charset="utf-8" X-Spam-Status: No, score=-8.4 required=5.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.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 (lipwig.vger.email [0.0.0.0]); Thu, 12 Oct 2023 15:25:37 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779590309453542415 X-GMAIL-MSGID: 1779590309453542415 |
Series |
net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf
|
|
Commit Message
Justin Stitt
Oct. 12, 2023, 10:25 p.m. UTC
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
ethtool_sprintf() is designed specifically for get_strings() usage.
Let's replace strncpy in favor of this dedicated helper function.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
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.
Found with: $ rg "strncpy\("
---
drivers/net/phy/nxp-tja11xx.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231012-strncpy-drivers-net-phy-nxp-tja11xx-c-99019080b1d4
Best regards,
--
Justin Stitt <justinstitt@google.com>
Comments
> - for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) { > - strncpy(data + i * ETH_GSTRING_LEN, > - tja11xx_hw_stats[i].string, ETH_GSTRING_LEN); > - } > + for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) > + ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string); > } I assume you are using "%s" because tja11xx_hw_stats[i].string cannot be trusted as a format string? Is this indicating we need an ethtool_puts() ? Andrew
On Fri, Oct 13, 2023 at 5:22 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > - for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) { > > - strncpy(data + i * ETH_GSTRING_LEN, > > - tja11xx_hw_stats[i].string, ETH_GSTRING_LEN); > > - } > > + for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) > > + ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string); > > } > > I assume you are using "%s" because tja11xx_hw_stats[i].string cannot > be trusted as a format string? Is this indicating we need an > ethtool_puts() ? Indeed, it would trigger a -Wformat-security warning. An ethtool_puts() would be useful for this situation. > > Andrew
On Fri, Oct 13, 2023 at 12:53:53PM -0700, Justin Stitt wrote: > On Fri, Oct 13, 2023 at 5:22 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > - for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) { > > > - strncpy(data + i * ETH_GSTRING_LEN, > > > - tja11xx_hw_stats[i].string, ETH_GSTRING_LEN); > > > - } > > > + for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) > > > + ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string); > > > } > > > > I assume you are using "%s" because tja11xx_hw_stats[i].string cannot > > be trusted as a format string? Is this indicating we need an > > ethtool_puts() ? > > Indeed, it would trigger a -Wformat-security warning. > > An ethtool_puts() would be useful for this situation. Hi Justin hyperv/netvsc_drv.c: ethtool_sprintf(&p, netvsc_stats[i].name); hyperv/netvsc_drv.c: ethtool_sprintf(&p, vf_stats[i].name); ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gstrings_priv_flags[i].flag_string); ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gl_gstrings_priv_flags[i].flag_string); ethernet/intel/ice/ice_ethtool.c: ethtool_sprintf(&p, ice_gstrings_priv_flags[i].name); ethernet/intel/igc/igc_ethtool.c: ethtool_sprintf(&p, igc_gstrings_stats[i].stat_string); ethernet/intel/ixgbe/ixgbe_ethtool.c: ethtool_sprintf(&p, ixgbe_gstrings_test[i]); ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_self_test[i].name); ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i + swap_off].name); ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i - swap_off].name); ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i].name); ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, txq_stat_names[j]); ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, xdpq_stat_names[j]); ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, rxq_stat_names[j]); ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, tls_stat_names[j]); ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); ethernet/brocade/bna/bnad_ethtool.c: ethtool_sprintf(&string, bnad_net_stats_strings[i]); ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_lif_stats_desc[i].name); ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_port_stats_desc[i].name); ethernet/hisilicon/hns/hns_dsaf_gmac.c: ethtool_sprintf(&buff, g_gmac_stats_string[i].desc); ethernet/hisilicon/hns/hns_dsaf_xgmac.c: ethtool_sprintf(&buff, g_xgmac_stats_string[i].desc); vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_dev_stats[i].desc); vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_driver_stats[i].desc); vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_dev_stats[i].desc); vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_driver_stats[i].desc); vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_global_stats[i].desc); It looks like there are enough potential users to justify adding it. Do you have the time and patience? Andrew
On Fri, Oct 13, 2023 at 1:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Fri, Oct 13, 2023 at 12:53:53PM -0700, Justin Stitt wrote: > > On Fri, Oct 13, 2023 at 5:22 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > - for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) { > > > > - strncpy(data + i * ETH_GSTRING_LEN, > > > > - tja11xx_hw_stats[i].string, ETH_GSTRING_LEN); > > > > - } > > > > + for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) > > > > + ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string); > > > > } > > > > > > I assume you are using "%s" because tja11xx_hw_stats[i].string cannot > > > be trusted as a format string? Is this indicating we need an > > > ethtool_puts() ? > > > > Indeed, it would trigger a -Wformat-security warning. > > > > An ethtool_puts() would be useful for this situation. > > Hi Justin > > hyperv/netvsc_drv.c: ethtool_sprintf(&p, netvsc_stats[i].name); > hyperv/netvsc_drv.c: ethtool_sprintf(&p, vf_stats[i].name); > ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gstrings_priv_flags[i].flag_string); > ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gl_gstrings_priv_flags[i].flag_string); > ethernet/intel/ice/ice_ethtool.c: ethtool_sprintf(&p, ice_gstrings_priv_flags[i].name); > ethernet/intel/igc/igc_ethtool.c: ethtool_sprintf(&p, igc_gstrings_stats[i].stat_string); > ethernet/intel/ixgbe/ixgbe_ethtool.c: ethtool_sprintf(&p, ixgbe_gstrings_test[i]); > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_self_test[i].name); > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i + swap_off].name); > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i - swap_off].name); > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i].name); > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, txq_stat_names[j]); > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, xdpq_stat_names[j]); > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, rxq_stat_names[j]); > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, tls_stat_names[j]); > ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); > ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); > ethernet/brocade/bna/bnad_ethtool.c: ethtool_sprintf(&string, bnad_net_stats_strings[i]); > ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_lif_stats_desc[i].name); > ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_port_stats_desc[i].name); > ethernet/hisilicon/hns/hns_dsaf_gmac.c: ethtool_sprintf(&buff, g_gmac_stats_string[i].desc); > ethernet/hisilicon/hns/hns_dsaf_xgmac.c: ethtool_sprintf(&buff, g_xgmac_stats_string[i].desc); > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_dev_stats[i].desc); > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_driver_stats[i].desc); > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_dev_stats[i].desc); > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_driver_stats[i].desc); > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_global_stats[i].desc); > Woah, are these all triggering -Wformat-security warnings? > It looks like there are enough potential users to justify adding > it. Do you have the time and patience? I do :) Should I create ethtool_puts() and then submit adoption patches for it in the same series? Or wait to hear back about how ethtool_puts() is received. > > Andrew Thanks Justin
On Fri, Oct 13, 2023 at 2:12 PM Justin Stitt <justinstitt@google.com> wrote: > > On Fri, Oct 13, 2023 at 1:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Fri, Oct 13, 2023 at 12:53:53PM -0700, Justin Stitt wrote: > > > On Fri, Oct 13, 2023 at 5:22 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) { > > > > > - strncpy(data + i * ETH_GSTRING_LEN, > > > > > - tja11xx_hw_stats[i].string, ETH_GSTRING_LEN); > > > > > - } > > > > > + for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) > > > > > + ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string); > > > > > } > > > > > > > > I assume you are using "%s" because tja11xx_hw_stats[i].string cannot > > > > be trusted as a format string? Is this indicating we need an > > > > ethtool_puts() ? > > > > > > Indeed, it would trigger a -Wformat-security warning. > > > > > > An ethtool_puts() would be useful for this situation. > > > > Hi Justin > > > > hyperv/netvsc_drv.c: ethtool_sprintf(&p, netvsc_stats[i].name); > > hyperv/netvsc_drv.c: ethtool_sprintf(&p, vf_stats[i].name); > > ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gstrings_priv_flags[i].flag_string); > > ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gl_gstrings_priv_flags[i].flag_string); > > ethernet/intel/ice/ice_ethtool.c: ethtool_sprintf(&p, ice_gstrings_priv_flags[i].name); > > ethernet/intel/igc/igc_ethtool.c: ethtool_sprintf(&p, igc_gstrings_stats[i].stat_string); > > ethernet/intel/ixgbe/ixgbe_ethtool.c: ethtool_sprintf(&p, ixgbe_gstrings_test[i]); > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_self_test[i].name); > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i + swap_off].name); > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i - swap_off].name); > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i].name); > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, txq_stat_names[j]); > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, xdpq_stat_names[j]); > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, rxq_stat_names[j]); > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, tls_stat_names[j]); > > ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); > > ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); > > ethernet/brocade/bna/bnad_ethtool.c: ethtool_sprintf(&string, bnad_net_stats_strings[i]); > > ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_lif_stats_desc[i].name); > > ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_port_stats_desc[i].name); > > ethernet/hisilicon/hns/hns_dsaf_gmac.c: ethtool_sprintf(&buff, g_gmac_stats_string[i].desc); > > ethernet/hisilicon/hns/hns_dsaf_xgmac.c: ethtool_sprintf(&buff, g_xgmac_stats_string[i].desc); > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_dev_stats[i].desc); > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_driver_stats[i].desc); > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_dev_stats[i].desc); > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_driver_stats[i].desc); > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_global_stats[i].desc); > > > > Woah, are these all triggering -Wformat-security warnings? Erhm, I guess -Wformat-security is turned off: ./scripts/Makefile.extrawarn +16: KBUILD_CFLAGS += -Wno-format-security Kees, what do you think about this warning and the semantics of: 1) ethtool_sprintf(&data, "%s", some[i].string); 2) ethtool_sprintf(&data, some[i].string); 3) ethtool_puts(&data, some[i].string); > > > It looks like there are enough potential users to justify adding > > it. Do you have the time and patience? > > I do :) > > Should I create ethtool_puts() and then submit adoption patches > for it in the same series? Or wait to hear back about how ethtool_puts() > is received. > > > > > Andrew > > Thanks > Justin
On Fri, Oct 13, 2023 at 02:23:34PM -0700, Justin Stitt wrote: > On Fri, Oct 13, 2023 at 2:12 PM Justin Stitt <justinstitt@google.com> wrote: > > > > On Fri, Oct 13, 2023 at 1:13 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > On Fri, Oct 13, 2023 at 12:53:53PM -0700, Justin Stitt wrote: > > > > On Fri, Oct 13, 2023 at 5:22 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > > > > > > - for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) { > > > > > > - strncpy(data + i * ETH_GSTRING_LEN, > > > > > > - tja11xx_hw_stats[i].string, ETH_GSTRING_LEN); > > > > > > - } > > > > > > + for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) > > > > > > + ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string); > > > > > > } > > > > > > > > > > I assume you are using "%s" because tja11xx_hw_stats[i].string cannot > > > > > be trusted as a format string? Is this indicating we need an > > > > > ethtool_puts() ? > > > > > > > > Indeed, it would trigger a -Wformat-security warning. > > > > > > > > An ethtool_puts() would be useful for this situation. > > > > > > Hi Justin > > > > > > hyperv/netvsc_drv.c: ethtool_sprintf(&p, netvsc_stats[i].name); > > > hyperv/netvsc_drv.c: ethtool_sprintf(&p, vf_stats[i].name); > > > ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gstrings_priv_flags[i].flag_string); > > > ethernet/intel/i40e/i40e_ethtool.c: ethtool_sprintf(&p, i40e_gl_gstrings_priv_flags[i].flag_string); > > > ethernet/intel/ice/ice_ethtool.c: ethtool_sprintf(&p, ice_gstrings_priv_flags[i].name); > > > ethernet/intel/igc/igc_ethtool.c: ethtool_sprintf(&p, igc_gstrings_stats[i].stat_string); > > > ethernet/intel/ixgbe/ixgbe_ethtool.c: ethtool_sprintf(&p, ixgbe_gstrings_test[i]); > > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_self_test[i].name); > > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i + swap_off].name); > > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i - swap_off].name); > > > ethernet/netronome/nfp/nfp_net_ethtool.c: ethtool_sprintf(&data, nfp_net_et_stats[i].name); > > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, txq_stat_names[j]); > > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, xdpq_stat_names[j]); > > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, rxq_stat_names[j]); > > > ethernet/fungible/funeth/funeth_ethtool.c: ethtool_sprintf(&p, tls_stat_names[j]); > > > ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); > > > ethernet/amazon/ena/ena_ethtool.c: ethtool_sprintf(&data, ena_stats->name); > > > ethernet/brocade/bna/bnad_ethtool.c: ethtool_sprintf(&string, bnad_net_stats_strings[i]); > > > ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_lif_stats_desc[i].name); > > > ethernet/pensando/ionic/ionic_stats.c: ethtool_sprintf(buf, ionic_port_stats_desc[i].name); > > > ethernet/hisilicon/hns/hns_dsaf_gmac.c: ethtool_sprintf(&buff, g_gmac_stats_string[i].desc); > > > ethernet/hisilicon/hns/hns_dsaf_xgmac.c: ethtool_sprintf(&buff, g_xgmac_stats_string[i].desc); > > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_dev_stats[i].desc); > > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_tq_driver_stats[i].desc); > > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_dev_stats[i].desc); > > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_rq_driver_stats[i].desc); > > > vmxnet3/vmxnet3_ethtool.c: ethtool_sprintf(&buf, vmxnet3_global_stats[i].desc); > > > > > > > Woah, are these all triggering -Wformat-security warnings? > > Erhm, I guess -Wformat-security is turned off: > > ./scripts/Makefile.extrawarn +16: > KBUILD_CFLAGS += -Wno-format-security Whee. This is a longer issue, but yes, it would be nice if we could get out of the way of enabling -Wformat-security again some day. > Kees, what do you think about this warning and the semantics of: > > 1) ethtool_sprintf(&data, "%s", some[i].string); > 2) ethtool_sprintf(&data, some[i].string); > 3) ethtool_puts(&data, some[i].string); I've been told that this whole ethtool API area is considered deprecated. If that holds, then I don't think it's worth adding new helpers to support it when ethtool_sprintf() is sufficient. Once you're done with the strncpy->ethtool_sprintf conversions I think it would be nice to have a single patch that fixes all of these "%s"-less instances to use "%s". (Doing per-driver fixes for that case seems just overly painful.)
On Thu, Oct 12, 2023 at 10:25:12PM +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 string > interfaces. > > ethtool_sprintf() is designed specifically for get_strings() usage. > Let's replace strncpy in favor of this dedicated helper function. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Yay for readability. :) Reviewed-by: Kees Cook <keescook@chromium.org>
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 12 Oct 2023 22:25:12 +0000 you wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > ethtool_sprintf() is designed specifically for get_strings() usage. > Let's replace strncpy in favor of this dedicated helper function. > > [...] Here is the summary with links: - net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf https://git.kernel.org/netdev/net-next/c/c3983d5e99b2 You are awesome, thank you!
> I've been told that this whole ethtool API area is considered > deprecated. If that holds, then I don't think it's worth adding new > helpers to support it when ethtool_sprintf() is sufficient. I think deprecated is too strong. The current API is not great, so maybe with time a new API will emerge. But given there are around 160 users of the API, probably over 100 drivers, it will be 20 years or more before all that hardware becomes obsolete and the drivers are removed. > Once you're done with the strncpy->ethtool_sprintf conversions I think > it would be nice to have a single patch that fixes all of these > "%s"-less instances to use "%s". (Doing per-driver fixes for that case > seems just overly painful.) I guess it is the same amount of effort to replace them with ethtool_puts()? checkpatch warns about seq_printf() which could be seq_puts(), so somebody thinks using puts is the right thing to do? Andrew
On Sat, Oct 14, 2023 at 03:55:41AM +0200, Andrew Lunn wrote: > > I've been told that this whole ethtool API area is considered > > deprecated. If that holds, then I don't think it's worth adding new > > helpers to support it when ethtool_sprintf() is sufficient. > > I think deprecated is too strong. The current API is not great, so > maybe with time a new API will emerge. But given there are around 160 > users of the API, probably over 100 drivers, it will be 20 years or > more before all that hardware becomes obsolete and the drivers are > removed. > > > Once you're done with the strncpy->ethtool_sprintf conversions I think > > it would be nice to have a single patch that fixes all of these > > "%s"-less instances to use "%s". (Doing per-driver fixes for that case > > seems just overly painful.) > > I guess it is the same amount of effort to replace them with > ethtool_puts()? Yup, right. If adding ethtool_puts() makes sense, then I totally agree.
On Sat, Oct 14, 2023 at 7:36 PM Kees Cook <keescook@chromium.org> wrote: > > On Sat, Oct 14, 2023 at 03:55:41AM +0200, Andrew Lunn wrote: > > > I've been told that this whole ethtool API area is considered > > > deprecated. If that holds, then I don't think it's worth adding new > > > helpers to support it when ethtool_sprintf() is sufficient. > > > > I think deprecated is too strong. The current API is not great, so > > maybe with time a new API will emerge. But given there are around 160 > > users of the API, probably over 100 drivers, it will be 20 years or > > more before all that hardware becomes obsolete and the drivers are > > removed. > > > > > Once you're done with the strncpy->ethtool_sprintf conversions I think > > > it would be nice to have a single patch that fixes all of these > > > "%s"-less instances to use "%s". (Doing per-driver fixes for that case > > > seems just overly painful.) > > > > I guess it is the same amount of effort to replace them with > > ethtool_puts()? > > Yup, right. If adding ethtool_puts() makes sense, then I totally agree. Thanks for the discussion here. I've sent a series [1] implementing ethtool_puts() and sending out a wave of replacements. [1]: https://lore.kernel.org/all/20231025-ethtool_puts_impl-v1-0-6a53a93d3b72@google.com/ > > -- > Kees Cook Thanks Justin
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c index b13e15310feb..a71399965142 100644 --- a/drivers/net/phy/nxp-tja11xx.c +++ b/drivers/net/phy/nxp-tja11xx.c @@ -414,10 +414,8 @@ static void tja11xx_get_strings(struct phy_device *phydev, u8 *data) { int i; - for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) { - strncpy(data + i * ETH_GSTRING_LEN, - tja11xx_hw_stats[i].string, ETH_GSTRING_LEN); - } + for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) + ethtool_sprintf(&data, "%s", tja11xx_hw_stats[i].string); } static void tja11xx_get_stats(struct phy_device *phydev,