* [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy @ 2023-10-05 0:30 Justin Stitt 2023-10-05 3:07 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Justin Stitt @ 2023-10-05 0:30 UTC (permalink / raw) To: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: netdev, linux-kernel, linux-hardening, Justin Stitt `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous interfaces. Let's opt for memcpy as we are copying strings into slices of length `ETH_GSTRING_LEN` within the `data` buffer. Other similar get_strings() implementations [2] [3] use memcpy(). Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/opa_vnic/opa_vnic_ethtool.c#L167 [2] Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c#L137 [3] 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/lan9303-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c index ee67adeb2cdb..665d69384b62 100644 --- a/drivers/net/dsa/lan9303-core.c +++ b/drivers/net/dsa/lan9303-core.c @@ -1013,8 +1013,8 @@ static void lan9303_get_strings(struct dsa_switch *ds, int port, return; for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) { - strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, - ETH_GSTRING_LEN); + memcpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, + ETH_GSTRING_LEN); } } --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231005-strncpy-drivers-net-dsa-lan9303-core-c-6386858e5c22 Best regards, -- Justin Stitt <justinstitt@google.com> ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy 2023-10-05 0:30 [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy Justin Stitt @ 2023-10-05 3:07 ` Kees Cook 2023-10-05 5:02 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Kees Cook @ 2023-10-05 3:07 UTC (permalink / raw) To: Justin Stitt Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux-hardening On Thu, Oct 05, 2023 at 12:30:18AM +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 > interfaces. > > Let's opt for memcpy as we are copying strings into slices of length > `ETH_GSTRING_LEN` within the `data` buffer. Other similar get_strings() > implementations [2] [3] use memcpy(). > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/opa_vnic/opa_vnic_ethtool.c#L167 [2] > Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c#L137 [3] > 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/lan9303-core.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > index ee67adeb2cdb..665d69384b62 100644 > --- a/drivers/net/dsa/lan9303-core.c > +++ b/drivers/net/dsa/lan9303-core.c > @@ -1013,8 +1013,8 @@ static void lan9303_get_strings(struct dsa_switch *ds, int port, > return; > > for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) { > - strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, > - ETH_GSTRING_LEN); > + memcpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, > + ETH_GSTRING_LEN); This won't work because lan9303_mib entries aren't ETH_GSTRING_LEN-long strings; they're string pointers: static const struct lan9303_mib_desc lan9303_mib[] = { { .offset = LAN9303_MAC_RX_BRDCST_CNT_0, .name = "RxBroad", }, So this really does need a strcpy-family function. And, I think the vnic_gstrings_stats and ipoib_gstrings_stats examples are actually buggy -- they're copying junk into userspace... I am reminded of this patch, which correctly uses strscpy_pad(): https://lore.kernel.org/lkml/20230718-net-dsa-strncpy-v1-1-e84664747713@google.com/ I think you want to do the same here, and use strscpy_pad(). And perhaps send some fixes for the other memcpy() users? (Have I mentioned I really dislike the get_strings() API?) -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy 2023-10-05 3:07 ` Kees Cook @ 2023-10-05 5:02 ` Kees Cook 2023-10-05 14:53 ` Alexander Lobakin 0 siblings, 1 reply; 5+ messages in thread From: Kees Cook @ 2023-10-05 5:02 UTC (permalink / raw) To: Justin Stitt Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux-hardening On Wed, Oct 04, 2023 at 08:07:55PM -0700, Kees Cook wrote: > On Thu, Oct 05, 2023 at 12:30:18AM +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 > > interfaces. > > > > Let's opt for memcpy as we are copying strings into slices of length > > `ETH_GSTRING_LEN` within the `data` buffer. Other similar get_strings() > > implementations [2] [3] use memcpy(). > > > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > > Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/opa_vnic/opa_vnic_ethtool.c#L167 [2] > > Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c#L137 [3] > > 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/lan9303-core.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > > index ee67adeb2cdb..665d69384b62 100644 > > --- a/drivers/net/dsa/lan9303-core.c > > +++ b/drivers/net/dsa/lan9303-core.c > > @@ -1013,8 +1013,8 @@ static void lan9303_get_strings(struct dsa_switch *ds, int port, > > return; > > > > for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) { > > - strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, > > - ETH_GSTRING_LEN); > > + memcpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, > > + ETH_GSTRING_LEN); > > This won't work because lan9303_mib entries aren't ETH_GSTRING_LEN-long > strings; they're string pointers: > > static const struct lan9303_mib_desc lan9303_mib[] = { > { .offset = LAN9303_MAC_RX_BRDCST_CNT_0, .name = "RxBroad", }, > > So this really does need a strcpy-family function. > > And, I think the vnic_gstrings_stats and ipoib_gstrings_stats examples > are actually buggy -- they're copying junk into userspace... > > I am reminded of this patch, which correctly uses strscpy_pad(): > https://lore.kernel.org/lkml/20230718-net-dsa-strncpy-v1-1-e84664747713@google.com/ > > I think you want to do the same here, and use strscpy_pad(). And perhaps > send some fixes for the other memcpy() users? Meh, I think it's not worth fixing the memcpy() users of this. This buggy pattern is very common, it seems: $ git grep 'data.*ETH_GSTRING_LEN' | grep memcpy | wc -l 47 -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy 2023-10-05 5:02 ` Kees Cook @ 2023-10-05 14:53 ` Alexander Lobakin 2023-10-05 18:58 ` Justin Stitt 0 siblings, 1 reply; 5+ messages in thread From: Alexander Lobakin @ 2023-10-05 14:53 UTC (permalink / raw) To: Kees Cook, Justin Stitt Cc: Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux-hardening From: Kees Cook <keescook@chromium.org> Date: Wed, 4 Oct 2023 22:02:00 -0700 > On Wed, Oct 04, 2023 at 08:07:55PM -0700, Kees Cook wrote: >> On Thu, Oct 05, 2023 at 12:30:18AM +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 >>> interfaces. >>> >>> Let's opt for memcpy as we are copying strings into slices of length >>> `ETH_GSTRING_LEN` within the `data` buffer. Other similar get_strings() >>> implementations [2] [3] use memcpy(). >>> >>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] >>> Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/opa_vnic/opa_vnic_ethtool.c#L167 [2] >>> Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c#L137 [3] >>> 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/lan9303-core.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c >>> index ee67adeb2cdb..665d69384b62 100644 >>> --- a/drivers/net/dsa/lan9303-core.c >>> +++ b/drivers/net/dsa/lan9303-core.c >>> @@ -1013,8 +1013,8 @@ static void lan9303_get_strings(struct dsa_switch *ds, int port, >>> return; >>> >>> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) { >>> - strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, >>> - ETH_GSTRING_LEN); >>> + memcpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, >>> + ETH_GSTRING_LEN); >> >> This won't work because lan9303_mib entries aren't ETH_GSTRING_LEN-long >> strings; they're string pointers: >> >> static const struct lan9303_mib_desc lan9303_mib[] = { >> { .offset = LAN9303_MAC_RX_BRDCST_CNT_0, .name = "RxBroad", }, >> >> So this really does need a strcpy-family function. >> >> And, I think the vnic_gstrings_stats and ipoib_gstrings_stats examples >> are actually buggy -- they're copying junk into userspace... >> >> I am reminded of this patch, which correctly uses strscpy_pad(): >> https://lore.kernel.org/lkml/20230718-net-dsa-strncpy-v1-1-e84664747713@google.com/ >> >> I think you want to do the same here, and use strscpy_pad(). And perhaps >> send some fixes for the other memcpy() users? > > Meh, I think it's not worth fixing the memcpy() users of this. This > buggy pattern is very common, it seems: > > $ git grep 'data.*ETH_GSTRING_LEN' | grep memcpy | wc -l > 47 We have ethtool_sprintf() precisely for the sake of filling the Ethtool statistics names. BTW this weird pattern "let's make the array of our stats names of fixed width (ETH_GSTRING_LEN), so that we could use memcpy() instead of strcpy()" was pretty common some time ago, no idea why, as it wastes memory for tons of \0 padding and provokes issues like the one you noticed here. > Thanks, Olek ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy 2023-10-05 14:53 ` Alexander Lobakin @ 2023-10-05 18:58 ` Justin Stitt 0 siblings, 0 replies; 5+ messages in thread From: Justin Stitt @ 2023-10-05 18:58 UTC (permalink / raw) To: Alexander Lobakin Cc: Kees Cook, Andrew Lunn, Florian Fainelli, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, linux-kernel, linux-hardening On Thu, Oct 5, 2023 at 7:54 AM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > From: Kees Cook <keescook@chromium.org> > Date: Wed, 4 Oct 2023 22:02:00 -0700 > > > On Wed, Oct 04, 2023 at 08:07:55PM -0700, Kees Cook wrote: > >> On Thu, Oct 05, 2023 at 12:30:18AM +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 > >>> interfaces. > >>> > >>> Let's opt for memcpy as we are copying strings into slices of length > >>> `ETH_GSTRING_LEN` within the `data` buffer. Other similar get_strings() > >>> implementations [2] [3] use memcpy(). > >>> > >>> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > >>> Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/opa_vnic/opa_vnic_ethtool.c#L167 [2] > >>> Link: https://elixir.bootlin.com/linux/v6.3/source/drivers/infiniband/ulp/ipoib/ipoib_ethtool.c#L137 [3] > >>> 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/lan9303-core.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c > >>> index ee67adeb2cdb..665d69384b62 100644 > >>> --- a/drivers/net/dsa/lan9303-core.c > >>> +++ b/drivers/net/dsa/lan9303-core.c > >>> @@ -1013,8 +1013,8 @@ static void lan9303_get_strings(struct dsa_switch *ds, int port, > >>> return; > >>> > >>> for (u = 0; u < ARRAY_SIZE(lan9303_mib); u++) { > >>> - strncpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, > >>> - ETH_GSTRING_LEN); > >>> + memcpy(data + u * ETH_GSTRING_LEN, lan9303_mib[u].name, > >>> + ETH_GSTRING_LEN); > >> > >> This won't work because lan9303_mib entries aren't ETH_GSTRING_LEN-long > >> strings; they're string pointers: > >> > >> static const struct lan9303_mib_desc lan9303_mib[] = { > >> { .offset = LAN9303_MAC_RX_BRDCST_CNT_0, .name = "RxBroad", }, > >> > >> So this really does need a strcpy-family function. > >> > >> And, I think the vnic_gstrings_stats and ipoib_gstrings_stats examples > >> are actually buggy -- they're copying junk into userspace... > >> > >> I am reminded of this patch, which correctly uses strscpy_pad(): > >> https://lore.kernel.org/lkml/20230718-net-dsa-strncpy-v1-1-e84664747713@google.com/ > >> > >> I think you want to do the same here, and use strscpy_pad(). And perhaps > >> send some fixes for the other memcpy() users? > > > > Meh, I think it's not worth fixing the memcpy() users of this. This > > buggy pattern is very common, it seems: > > > > $ git grep 'data.*ETH_GSTRING_LEN' | grep memcpy | wc -l > > 47 > > We have ethtool_sprintf() precisely for the sake of filling the Ethtool > statistics names. > > BTW this weird pattern "let's make the array of our stats names of fixed > width (ETH_GSTRING_LEN), so that we could use memcpy() instead of > strcpy()" was pretty common some time ago, no idea why, as it wastes > memory for tons of \0 padding and provokes issues like the one you > noticed here. Just sent a v2 using ethtool_sprintf(). I'd appreciate some feedback on if I used it correctly. > > > > > Thanks, > Olek [v2]: https://lore.kernel.org/r/20231005-strncpy-drivers-net-dsa-lan9303-core-c-v2-1-feb452a532db@google.com Thanks Justin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-05 18:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-05 0:30 [PATCH] net: dsa: lan9303: replace deprecated strncpy with memcpy Justin Stitt 2023-10-05 3:07 ` Kees Cook 2023-10-05 5:02 ` Kees Cook 2023-10-05 14:53 ` Alexander Lobakin 2023-10-05 18:58 ` Justin Stitt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).