| Message ID | 20231009-strncpy-drivers-net-dsa-realtek-rtl8365mb-c-v1-1-0537fe9fb08c@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e5f061d5e340fefc663cacfe8f42f149d55bdb53 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: rtl8365mb: replace deprecated strncpy with ethtool_sprintf | expand |
On 10/9/23 15:43, 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 more robust and easier to > understand interface. > > 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> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
On Mon, Oct 09, 2023 at 10:43:59PM +0000, Justin Stitt wrote: > [You don't often get email from justinstitt@google.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > `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 more robust and easier to > understand interface. > > 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> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > --- > Note: build-tested only. > --- > drivers/net/dsa/realtek/rtl8365mb.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index 41ea3b5a42b1..d171c18dd354 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -1303,8 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, int port, u32 stringset > > for (i = 0; i < RTL8365MB_MIB_END; i++) { > struct rtl8365mb_mib_counter *mib = &rtl8365mb_mib_counters[i]; > - > - strncpy(data + i * ETH_GSTRING_LEN, mib->name, ETH_GSTRING_LEN); > + ethtool_sprintf(&data, "%s", mib->name); > } > } > > > --- > base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 > change-id: 20231009-strncpy-drivers-net-dsa-realtek-rtl8365mb-c-bb106e4c110c > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
Hello Justin, On Mon, Oct 09, 2023 at 10:43:59PM +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 more robust and easier to > understand interface. > > 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. > --- > drivers/net/dsa/realtek/rtl8365mb.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > index 41ea3b5a42b1..d171c18dd354 100644 > --- a/drivers/net/dsa/realtek/rtl8365mb.c > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > @@ -1303,8 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, int port, u32 stringset > > for (i = 0; i < RTL8365MB_MIB_END; i++) { > struct rtl8365mb_mib_counter *mib = &rtl8365mb_mib_counters[i]; > - > - strncpy(data + i * ETH_GSTRING_LEN, mib->name, ETH_GSTRING_LEN); > + ethtool_sprintf(&data, "%s", mib->name); Is there any particular reason why you opted for the "%s" printf format specifier when you could have simply given mib->name as the single argument? This comment applies to all the ethtool_sprintf() patches you've submitted.
On Tue, Oct 10, 2023 at 4:07 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > Hello Justin, > > On Mon, Oct 09, 2023 at 10:43:59PM +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 more robust and easier to > > understand interface. > > > > 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. > > --- > > drivers/net/dsa/realtek/rtl8365mb.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > > index 41ea3b5a42b1..d171c18dd354 100644 > > --- a/drivers/net/dsa/realtek/rtl8365mb.c > > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > > @@ -1303,8 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, int port, u32 stringset > > > > for (i = 0; i < RTL8365MB_MIB_END; i++) { > > struct rtl8365mb_mib_counter *mib = &rtl8365mb_mib_counters[i]; > > - > > - strncpy(data + i * ETH_GSTRING_LEN, mib->name, ETH_GSTRING_LEN); > > + ethtool_sprintf(&data, "%s", mib->name); > > Is there any particular reason why you opted for the "%s" printf format > specifier when you could have simply given mib->name as the single > argument? This comment applies to all the ethtool_sprintf() patches > you've submitted. Yeah, it causes a -Wformat-security warning for me. I briefly mentioned it in one of my first patches like this [1]. [1]: https://lore.kernel.org/all/20231005-strncpy-drivers-net-dsa-lan9303-core-c-v2-1-feb452a532db@google.com/
On Tue, Oct 10, 2023 at 10:36 AM Justin Stitt <justinstitt@google.com> wrote: > > On Tue, Oct 10, 2023 at 4:07 AM Vladimir Oltean <olteanv@gmail.com> wrote: > > > > Hello Justin, > > > > On Mon, Oct 09, 2023 at 10:43:59PM +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 more robust and easier to > > > understand interface. > > > > > > 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. > > > --- > > > drivers/net/dsa/realtek/rtl8365mb.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > > > index 41ea3b5a42b1..d171c18dd354 100644 > > > --- a/drivers/net/dsa/realtek/rtl8365mb.c > > > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > > > @@ -1303,8 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, int port, u32 stringset > > > > > > for (i = 0; i < RTL8365MB_MIB_END; i++) { > > > struct rtl8365mb_mib_counter *mib = &rtl8365mb_mib_counters[i]; > > > - > > > - strncpy(data + i * ETH_GSTRING_LEN, mib->name, ETH_GSTRING_LEN); > > > + ethtool_sprintf(&data, "%s", mib->name); > > > > Is there any particular reason why you opted for the "%s" printf format > > specifier when you could have simply given mib->name as the single > > argument? This comment applies to all the ethtool_sprintf() patches > > you've submitted. > > Yeah, it causes a -Wformat-security warning for me. I briefly mentioned it > in one of my first patches like this [1]. For more context, here's some warnings in the wild: https://lore.kernel.org/netdev/20231003183603.3887546-3-jesse.brandeburg@intel.com/ > > [1]: https://lore.kernel.org/all/20231005-strncpy-drivers-net-dsa-lan9303-core-c-v2-1-feb452a532db@google.com/
On Tue, Oct 10, 2023 at 02:48:07PM -0700, Justin Stitt wrote: > On Tue, Oct 10, 2023 at 10:36 AM Justin Stitt <justinstitt@google.com> wrote: > > > Is there any particular reason why you opted for the "%s" printf format > > > specifier when you could have simply given mib->name as the single > > > argument? This comment applies to all the ethtool_sprintf() patches > > > you've submitted. > > > > Yeah, it causes a -Wformat-security warning for me. I briefly mentioned it > > in one of my first patches like this [1]. > > For more context, here's some warnings in the wild: > https://lore.kernel.org/netdev/20231003183603.3887546-3-jesse.brandeburg@intel.com/ > > > > > [1]: https://lore.kernel.org/all/20231005-strncpy-drivers-net-dsa-lan9303-core-c-v2-1-feb452a532db@google.com/ Yeah, ok. It's a false positive warning, but I guess it would be too hard for the compiler to figure that out.
On Mon, Oct 09, 2023 at 10:43:59PM +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 more robust and easier to > understand interface. > > 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> > --- Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
On Tue, Oct 10, 2023 at 02:07:17PM +0300, Vladimir Oltean wrote: > Hello Justin, > > On Mon, Oct 09, 2023 at 10:43:59PM +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 more robust and easier to > > understand interface. > > > > 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. > > --- > > drivers/net/dsa/realtek/rtl8365mb.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c > > index 41ea3b5a42b1..d171c18dd354 100644 > > --- a/drivers/net/dsa/realtek/rtl8365mb.c > > +++ b/drivers/net/dsa/realtek/rtl8365mb.c > > @@ -1303,8 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, int port, u32 stringset > > > > for (i = 0; i < RTL8365MB_MIB_END; i++) { > > struct rtl8365mb_mib_counter *mib = &rtl8365mb_mib_counters[i]; > > - > > - strncpy(data + i * ETH_GSTRING_LEN, mib->name, ETH_GSTRING_LEN); > > + ethtool_sprintf(&data, "%s", mib->name); > > Is there any particular reason why you opted for the "%s" printf format > specifier when you could have simply given mib->name as the single > argument? This comment applies to all the ethtool_sprintf() patches > you've submitted. The primary reason is that without the "%s", any format specifiers in mib->name will be processed by sprintf(), which could lead to very unexpected results. One never wants to just pass a string directly to the sprintf-family of functions for this reason. "%s" is needed to keep things safe. -Kees
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 09 Oct 2023 22:43:59 +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 more robust and easier to > understand interface. > > [...] Here is the summary with links: - net: dsa: realtek: rtl8365mb: replace deprecated strncpy with ethtool_sprintf https://git.kernel.org/netdev/net-next/c/e5f061d5e340 You are awesome, thank you!
diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c index 41ea3b5a42b1..d171c18dd354 100644 --- a/drivers/net/dsa/realtek/rtl8365mb.c +++ b/drivers/net/dsa/realtek/rtl8365mb.c @@ -1303,8 +1303,7 @@ static void rtl8365mb_get_strings(struct dsa_switch *ds, int port, u32 stringset for (i = 0; i < RTL8365MB_MIB_END; i++) { struct rtl8365mb_mib_counter *mib = &rtl8365mb_mib_counters[i]; - - strncpy(data + i * ETH_GSTRING_LEN, mib->name, ETH_GSTRING_LEN); + ethtool_sprintf(&data, "%s", mib->name); } }
`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 more robust and easier to understand interface. 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. --- drivers/net/dsa/realtek/rtl8365mb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231009-strncpy-drivers-net-dsa-realtek-rtl8365mb-c-bb106e4c110c Best regards, -- Justin Stitt <justinstitt@google.com>