| Message ID | 20231010-netdev-replace-strncpy-resend-as-series-v1-0-caf9f0f2f021@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: intel: replace deprecated strncpy uses | expand |
On 10/10/2023 3:26 PM, Justin Stitt wrote: > Hi, > > This series aims to eliminate uses of strncpy() as it is a deprecated > interface [1] with many viable replacements available. > > Predominantly, strscpy() is the go-to replacement as it guarantees > NUL-termination on the destination buffer (which strncpy does not). With > that being said, I did not identify any buffer overread problems as the > size arguments were carefully measured to leave room for trailing > NUL-bytes. Nonetheless, we should favor more robust and less ambiguous > interfaces. > > Previously, each of these patches was sent individually at: > 1) https://lore.kernel.org/all/20231009-strncpy-drivers-net-ethernet-intel-e100-c-v1-1-ca0ff96868a3@google.com/ > 2) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-e1000-e1000_main-c-v1-1-b1d64581f983@google.com/ > 3) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-fm10k-fm10k_ethtool-c-v1-1-dbdc4570c5a6@google.com/ > 4) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-i40e-i40e_ddp-c-v1-1-f01a23394eab@google.com/ > 5) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igb-igb_main-c-v1-1-d796234a8abf@google.com/ > 6) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-v1-1-69ccfb2c2aa5@google.com/ > 7) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igc-igc_main-c-v1-1-f1f507ecc476@google.com/ > > Consider these dead as this series is their new home :) > > I found all these instances with: $ rg "strncpy\(" > > This series may collide in a not-so-nice way with [3]. This series can > go in after that one with a rebase. I'll send a v2 if necessary. > > [3]: https://lore.kernel.org/netdev/20231003183603.3887546-1-jesse.brandeburg@intel.com/ > > 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 > Signed-off-by: Justin Stitt <justinstitt@google.com> Thanks Justin for fixing all these! For the series: Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> PS: have you considered adding a script to scripts/coccinelle/api which might catch and try to fix future (ab)users of strncpy?
On Tue, Oct 10, 2023 at 4:19 PM Jesse Brandeburg <jesse.brandeburg@intel.com> wrote: > > On 10/10/2023 3:26 PM, Justin Stitt wrote: > > Hi, > > > > This series aims to eliminate uses of strncpy() as it is a deprecated > > interface [1] with many viable replacements available. > > > > Predominantly, strscpy() is the go-to replacement as it guarantees > > NUL-termination on the destination buffer (which strncpy does not). With > > that being said, I did not identify any buffer overread problems as the > > size arguments were carefully measured to leave room for trailing > > NUL-bytes. Nonetheless, we should favor more robust and less ambiguous > > interfaces. > > > > Previously, each of these patches was sent individually at: > > 1) https://lore.kernel.org/all/20231009-strncpy-drivers-net-ethernet-intel-e100-c-v1-1-ca0ff96868a3@google.com/ > > 2) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-e1000-e1000_main-c-v1-1-b1d64581f983@google.com/ > > 3) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-fm10k-fm10k_ethtool-c-v1-1-dbdc4570c5a6@google.com/ > > 4) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-i40e-i40e_ddp-c-v1-1-f01a23394eab@google.com/ > > 5) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igb-igb_main-c-v1-1-d796234a8abf@google.com/ > > 6) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-v1-1-69ccfb2c2aa5@google.com/ > > 7) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igc-igc_main-c-v1-1-f1f507ecc476@google.com/ > > > > Consider these dead as this series is their new home :) > > > > I found all these instances with: $ rg "strncpy\(" > > > > This series may collide in a not-so-nice way with [3]. This series can > > go in after that one with a rebase. I'll send a v2 if necessary. > > > > [3]: https://lore.kernel.org/netdev/20231003183603.3887546-1-jesse.brandeburg@intel.com/ > > > > 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 > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > Thanks Justin for fixing all these! > > For the series: > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > > PS: have you considered adding a script to scripts/coccinelle/api which > might catch and try to fix future (ab)users of strncpy? There is a checkpatch routine for it. Also, the docs are littered with aversions to strncpy. With that being said, I would not be opposed to adding more checks, though. Once I'm more caught up on all the outstanding strncpy uses, I'll look into adding some coccinelle support. > Thanks Justin
On Tue, Oct 10, 2023 at 04:22:44PM -0700, Justin Stitt wrote: > On Tue, Oct 10, 2023 at 4:19 PM Jesse Brandeburg > <jesse.brandeburg@intel.com> wrote: > > > > On 10/10/2023 3:26 PM, Justin Stitt wrote: > > > Hi, > > > > > > This series aims to eliminate uses of strncpy() as it is a deprecated > > > interface [1] with many viable replacements available. > > > > > > Predominantly, strscpy() is the go-to replacement as it guarantees > > > NUL-termination on the destination buffer (which strncpy does not). With > > > that being said, I did not identify any buffer overread problems as the > > > size arguments were carefully measured to leave room for trailing > > > NUL-bytes. Nonetheless, we should favor more robust and less ambiguous > > > interfaces. > > > > > > Previously, each of these patches was sent individually at: > > > 1) https://lore.kernel.org/all/20231009-strncpy-drivers-net-ethernet-intel-e100-c-v1-1-ca0ff96868a3@google.com/ > > > 2) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-e1000-e1000_main-c-v1-1-b1d64581f983@google.com/ > > > 3) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-fm10k-fm10k_ethtool-c-v1-1-dbdc4570c5a6@google.com/ > > > 4) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-i40e-i40e_ddp-c-v1-1-f01a23394eab@google.com/ > > > 5) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igb-igb_main-c-v1-1-d796234a8abf@google.com/ > > > 6) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-v1-1-69ccfb2c2aa5@google.com/ > > > 7) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igc-igc_main-c-v1-1-f1f507ecc476@google.com/ > > > > > > Consider these dead as this series is their new home :) > > > > > > I found all these instances with: $ rg "strncpy\(" > > > > > > This series may collide in a not-so-nice way with [3]. This series can > > > go in after that one with a rebase. I'll send a v2 if necessary. > > > > > > [3]: https://lore.kernel.org/netdev/20231003183603.3887546-1-jesse.brandeburg@intel.com/ > > > > > > 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 > > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > > > Thanks Justin for fixing all these! > > > > For the series: > > Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > > > > PS: have you considered adding a script to scripts/coccinelle/api which > > might catch and try to fix future (ab)users of strncpy? > > There is a checkpatch routine for it. Also, the docs are littered with > aversions to strncpy. With that being said, I would not be opposed > to adding more checks, though. > > Once I'm more caught up on all the outstanding strncpy uses, > I'll look into adding some coccinelle support. Coccinelle for strncpy is difficult since each set of callers tends to need careful examination. But the good news here is that at the current rate, the kernel may be strncpy-free pretty soon. :)
On Tue, Oct 10, 2023 at 10:26:53PM +0000, Justin Stitt wrote: > Hi, > > This series aims to eliminate uses of strncpy() as it is a deprecated > interface [1] with many viable replacements available. > > Predominantly, strscpy() is the go-to replacement as it guarantees > NUL-termination on the destination buffer (which strncpy does not). With > that being said, I did not identify any buffer overread problems as the > size arguments were carefully measured to leave room for trailing > NUL-bytes. Nonetheless, we should favor more robust and less ambiguous > interfaces. > > Previously, each of these patches was sent individually at: > 1) https://lore.kernel.org/all/20231009-strncpy-drivers-net-ethernet-intel-e100-c-v1-1-ca0ff96868a3@google.com/ > 2) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-e1000-e1000_main-c-v1-1-b1d64581f983@google.com/ > 3) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-fm10k-fm10k_ethtool-c-v1-1-dbdc4570c5a6@google.com/ > 4) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-i40e-i40e_ddp-c-v1-1-f01a23394eab@google.com/ > 5) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igb-igb_main-c-v1-1-d796234a8abf@google.com/ > 6) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-v1-1-69ccfb2c2aa5@google.com/ > 7) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igc-igc_main-c-v1-1-f1f507ecc476@google.com/ > > Consider these dead as this series is their new home :) > > I found all these instances with: $ rg "strncpy\(" > > This series may collide in a not-so-nice way with [3]. This series can > go in after that one with a rebase. I'll send a v2 if necessary. > > [3]: https://lore.kernel.org/netdev/20231003183603.3887546-1-jesse.brandeburg@intel.com/ > > 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 > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Justin Stitt (7): > e100: replace deprecated strncpy with strscpy > e1000: replace deprecated strncpy with strscpy > fm10k: replace deprecated strncpy with strscpy > i40e: use scnprintf over strncpy+strncat > igb: replace deprecated strncpy with strscpy > igbvf: replace deprecated strncpy with strscpy > igc: replace deprecated strncpy with strscpy These all look good to me. Thanks for the careful analysis! Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
On 10/10/2023 3:26 PM, Justin Stitt wrote: > Hi, > > This series aims to eliminate uses of strncpy() as it is a deprecated > interface [1] with many viable replacements available. > > Predominantly, strscpy() is the go-to replacement as it guarantees > NUL-termination on the destination buffer (which strncpy does not). With > that being said, I did not identify any buffer overread problems as the > size arguments were carefully measured to leave room for trailing > NUL-bytes. Nonetheless, we should favor more robust and less ambiguous > interfaces. > > Previously, each of these patches was sent individually at: > 1) https://lore.kernel.org/all/20231009-strncpy-drivers-net-ethernet-intel-e100-c-v1-1-ca0ff96868a3@google.com/ > 2) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-e1000-e1000_main-c-v1-1-b1d64581f983@google.com/ > 3) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-fm10k-fm10k_ethtool-c-v1-1-dbdc4570c5a6@google.com/ > 4) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-i40e-i40e_ddp-c-v1-1-f01a23394eab@google.com/ > 5) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igb-igb_main-c-v1-1-d796234a8abf@google.com/ > 6) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-v1-1-69ccfb2c2aa5@google.com/ > 7) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igc-igc_main-c-v1-1-f1f507ecc476@google.com/ > > Consider these dead as this series is their new home :) > > I found all these instances with: $ rg "strncpy\(" > > This series may collide in a not-so-nice way with [3]. This series can > go in after that one with a rebase. I'll send a v2 if necessary. > I'm working to apply these to the Intel Wired LAN dev-queue now, and I'll see how bad it is. I will ping you if I need a rebased version. Thanks, Jake
Hi, This series aims to eliminate uses of strncpy() as it is a deprecated interface [1] with many viable replacements available. Predominantly, strscpy() is the go-to replacement as it guarantees NUL-termination on the destination buffer (which strncpy does not). With that being said, I did not identify any buffer overread problems as the size arguments were carefully measured to leave room for trailing NUL-bytes. Nonetheless, we should favor more robust and less ambiguous interfaces. Previously, each of these patches was sent individually at: 1) https://lore.kernel.org/all/20231009-strncpy-drivers-net-ethernet-intel-e100-c-v1-1-ca0ff96868a3@google.com/ 2) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-e1000-e1000_main-c-v1-1-b1d64581f983@google.com/ 3) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-fm10k-fm10k_ethtool-c-v1-1-dbdc4570c5a6@google.com/ 4) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-i40e-i40e_ddp-c-v1-1-f01a23394eab@google.com/ 5) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igb-igb_main-c-v1-1-d796234a8abf@google.com/ 6) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-v1-1-69ccfb2c2aa5@google.com/ 7) https://lore.kernel.org/all/20231010-strncpy-drivers-net-ethernet-intel-igc-igc_main-c-v1-1-f1f507ecc476@google.com/ Consider these dead as this series is their new home :) I found all these instances with: $ rg "strncpy\(" This series may collide in a not-so-nice way with [3]. This series can go in after that one with a rebase. I'll send a v2 if necessary. [3]: https://lore.kernel.org/netdev/20231003183603.3887546-1-jesse.brandeburg@intel.com/ 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 Signed-off-by: Justin Stitt <justinstitt@google.com> --- Justin Stitt (7): e100: replace deprecated strncpy with strscpy e1000: replace deprecated strncpy with strscpy fm10k: replace deprecated strncpy with strscpy i40e: use scnprintf over strncpy+strncat igb: replace deprecated strncpy with strscpy igbvf: replace deprecated strncpy with strscpy igc: replace deprecated strncpy with strscpy drivers/net/ethernet/intel/e100.c | 2 +- drivers/net/ethernet/intel/e1000/e1000_main.c | 2 +- drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 8 ++++---- drivers/net/ethernet/intel/i40e/i40e_ddp.c | 7 +++---- drivers/net/ethernet/intel/igb/igb_main.c | 2 +- drivers/net/ethernet/intel/igbvf/netdev.c | 2 +- drivers/net/ethernet/intel/igc/igc_main.c | 2 +- 7 files changed, 12 insertions(+), 13 deletions(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231010-netdev-replace-strncpy-resend-as-series-dee90d0c63bd Best regards, -- Justin Stitt <justinstitt@google.com>