| Message ID | 20230718-net-dsa-strncpy-v1-1-e84664747713@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: remove deprecated strncpy | expand |
On July 17, 2023 5:04:19 PM PDT, justinstitt@google.com wrote: >`strncpy` is deprecated for use on NUL-terminated destination strings [1]. > >Even call sites utilizing length-bounded destination buffers should >switch over to using `strtomem` or `strtomem_pad`. In this case, >however, the compiler is unable to determine the size of the `data` >buffer which renders `strtomem` unusable. Due to this, `strscpy` >should be used. > >It should be noted that most call sites already zero-initialize the >destination buffer. However, I've opted to use `strscpy_pad` to maintain >the same exact behavior that `strncpy` produced (zero-padded tail up to >`len`). > >Also see [3]. > >[1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings >[2]: elixir.bootlin.com/linux/v6.3/source/net/ethtool/ioctl.c#L1944 >[3]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > >Link: https://github.com/KSPP/linux/issues/90 >Signed-off-by: Justin Stitt <justinstitt@google.com> This looks fine to me. I think the _pad variant is overkill (this region is already zero-initialized[1]), but it's a reasonable precaution for robustness. Honestly I find the entire get_strings API to be very fragile given the lack of passing the length of the buffer, instead depending on the string set length lookups in each callback, but refactoring that looks like a ton of work for an uncertain benefit. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees [1] https://elixir.bootlin.com/linux/v6.3/source/net/ethtool/ioctl.c#L1944
On Mon, Jul 17, 2023 at 5:04 PM <justinstitt@google.com> wrote: > > `strncpy` is deprecated for use on NUL-terminated destination strings [1]. > > Even call sites utilizing length-bounded destination buffers should > switch over to using `strtomem` or `strtomem_pad`. In this case, > however, the compiler is unable to determine the size of the `data` > buffer which renders `strtomem` unusable. Due to this, `strscpy` > should be used. > > It should be noted that most call sites already zero-initialize the > destination buffer. However, I've opted to use `strscpy_pad` to maintain > the same exact behavior that `strncpy` produced (zero-padded tail up to > `len`). > > Also see [3]. > > [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings > [2]: elixir.bootlin.com/linux/v6.3/source/net/ethtool/ioctl.c#L1944 > [3]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html > > Link: https://github.com/KSPP/linux/issues/90 > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > net/dsa/slave.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 527b1d576460..c9f77b7e5895 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1056,10 +1056,10 @@ static void dsa_slave_get_strings(struct net_device *dev, > if (stringset == ETH_SS_STATS) { > int len = ETH_GSTRING_LEN; > > - strncpy(data, "tx_packets", len); > - strncpy(data + len, "tx_bytes", len); > - strncpy(data + 2 * len, "rx_packets", len); > - strncpy(data + 3 * len, "rx_bytes", len); > + strscpy_pad(data, "tx_packets", len); > + strscpy_pad(data + len, "tx_bytes", len); > + strscpy_pad(data + 2 * len, "rx_packets", len); > + strscpy_pad(data + 3 * len, "rx_bytes", len); Thanks for the patch! Consider adding a #include <linux/string.h> so that we stop having such an indirect dependency in this TU. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > if (ds->ops->get_strings) > ds->ops->get_strings(ds, dp->index, stringset, > data + 4 * len); > > --- > base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c > change-id: 20230717-net-dsa-strncpy-844ca1111eb2 > > Best regards, > -- > Justin Stitt <justinstitt@google.com> >
On Tue, 18 Jul 2023 11:05:23 -0700 Kees Cook wrote: > Honestly I find the entire get_strings API to be very fragile given > the lack of passing the length of the buffer, instead depending on > the string set length lookups in each callback, but refactoring that > looks like a ton of work for an uncertain benefit. We have been adding better APIs for long term, and a print helper short term - ethtool_sprintf(). Should we use ethtool_sprintf() here?
On Tue, Jul 18, 2023 at 12:11:16PM -0700, Jakub Kicinski wrote: > On Tue, 18 Jul 2023 11:05:23 -0700 Kees Cook wrote: > > Honestly I find the entire get_strings API to be very fragile given > > the lack of passing the length of the buffer, instead depending on > > the string set length lookups in each callback, but refactoring that > > looks like a ton of work for an uncertain benefit. > > We have been adding better APIs for long term, and a print helper short > term - ethtool_sprintf(). Should we use ethtool_sprintf() here? I was wondering about that as well. There is no variable expansion in most cases, so the vsnprintf() is a waste of time. Maybe we should actually add another helper: ethtool_name_cpy(u8 **data, unsigned int index, const char *name); Then over the next decade, slowly convert all drivers to it. And then eventually replace the u8 with a struct including the length. The netlink API is a bit better. It is one kAPI call which does everything, and it holds RTNL. So it is less likely the number of statistics will change between the calls into the driver. Andrew
On Tue, 18 Jul 2023 21:31:04 +0200 Andrew Lunn wrote: > On Tue, Jul 18, 2023 at 12:11:16PM -0700, Jakub Kicinski wrote: > > On Tue, 18 Jul 2023 11:05:23 -0700 Kees Cook wrote: > > > Honestly I find the entire get_strings API to be very fragile given > > > the lack of passing the length of the buffer, instead depending on > > > the string set length lookups in each callback, but refactoring that > > > looks like a ton of work for an uncertain benefit. > > > > We have been adding better APIs for long term, and a print helper short > > term - ethtool_sprintf(). Should we use ethtool_sprintf() here? > > I was wondering about that as well. There is no variable expansion in > most cases, so the vsnprintf() is a waste of time. > > Maybe we should actually add another helper: > > ethtool_name_cpy(u8 **data, unsigned int index, const char *name); I wasn't sure if vsnprintf() is costly enough to bother, but SG. Probably without the "unsigned int index", since the ethtool_sprintf() API updates the first argument for the caller. > Then over the next decade, slowly convert all drivers to it. And then > eventually replace the u8 with a struct including the length. > > The netlink API is a bit better. It is one kAPI call which does > everything, and it holds RTNL. So it is less likely the number of > statistics will change between the calls into the driver.
From: Andrew Lunn > Sent: 18 July 2023 20:31 ... > Maybe we should actually add another helper: > > ethtool_name_cpy(u8 **data, unsigned int index, const char *name); > > Then over the next decade, slowly convert all drivers to it. And then > eventually replace the u8 with a struct including the length. Define the structure with the length from the start. Add a wrapper that allows the length to be absent. (Either ignoring the length or using 0/infinity to mean no length.) Then you don't need to visit everywhere twice - just some places. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 527b1d576460..c9f77b7e5895 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1056,10 +1056,10 @@ static void dsa_slave_get_strings(struct net_device *dev, if (stringset == ETH_SS_STATS) { int len = ETH_GSTRING_LEN; - strncpy(data, "tx_packets", len); - strncpy(data + len, "tx_bytes", len); - strncpy(data + 2 * len, "rx_packets", len); - strncpy(data + 3 * len, "rx_bytes", len); + strscpy_pad(data, "tx_packets", len); + strscpy_pad(data + len, "tx_bytes", len); + strscpy_pad(data + 2 * len, "rx_packets", len); + strscpy_pad(data + 3 * len, "rx_bytes", len); if (ds->ops->get_strings) ds->ops->get_strings(ds, dp->index, stringset, data + 4 * len);
`strncpy` is deprecated for use on NUL-terminated destination strings [1]. Even call sites utilizing length-bounded destination buffers should switch over to using `strtomem` or `strtomem_pad`. In this case, however, the compiler is unable to determine the size of the `data` buffer which renders `strtomem` unusable. Due to this, `strscpy` should be used. It should be noted that most call sites already zero-initialize the destination buffer. However, I've opted to use `strscpy_pad` to maintain the same exact behavior that `strncpy` produced (zero-padded tail up to `len`). Also see [3]. [1]: www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [2]: elixir.bootlin.com/linux/v6.3/source/net/ethtool/ioctl.c#L1944 [3]: manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html Link: https://github.com/KSPP/linux/issues/90 Signed-off-by: Justin Stitt <justinstitt@google.com> --- net/dsa/slave.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) --- base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c change-id: 20230717-net-dsa-strncpy-844ca1111eb2 Best regards,