| Message ID | 20231012-strncpy-drivers-net-phy-nxp-tja11xx-c-v1-1-5ad6c9dff5c4@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c3983d5e99b2f11839451fc264a7321163e9e893 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf | expand |
> - 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,
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>