[PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Justin Stitt]

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(-)

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 <justinstitt@google.com>

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Andrew Lunn]

> -	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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Justin Stitt]

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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Andrew Lunn]

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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Justin Stitt]

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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Justin Stitt]

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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Kees Cook]

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.)

-- 
Kees Cook

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Andrew Lunn]

> 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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Kees Cook]

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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[Justin Stitt]

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

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[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: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Yay for readability. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] net: phy: tja11xx: replace deprecated strncpy with ethtool_sprintf

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html