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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
Found with: $ rg "strncpy\("
---
drivers/net/phy/nxp-tja11xx.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
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,
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231012-strncpy-drivers-net-phy-nxp-tja11xx-c-99019080b1d4
Best regards,
--
Justin Stitt <[email protected]>
> - 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>
> On Fri, Oct 13, 2023 at 1:13 PM Andrew Lunn <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> >
> > On Fri, Oct 13, 2023 at 1:13 PM Andrew Lunn <[email protected]> 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 <[email protected]> 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.)
--
Kees Cook
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
Yay for readability. :)
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
Hello:
This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
> 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.
--
Kees Cook
On Sat, Oct 14, 2023 at 7:36 PM Kees Cook <[email protected]> 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/[email protected]/
>
> --
> Kees Cook
Thanks
Justin