| Message ID | 20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-v1-1-69ccfb2c2aa5@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | igbvf: replace deprecated strncpy with strscpy | expand |
On 10/10/2023 2:12 PM, 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. > > We expect netdev->name to be NUL-terminated based on its usage with > `strlen` and format strings: > | if (strlen(netdev->name) < (IFNAMSIZ - 5)) { > | sprintf(adapter->tx_ring->name, "%s-tx-0", netdev->name); > > Moreover, we do not need NUL-padding as netdev is already > zero-allocated: > | netdev = alloc_etherdev(sizeof(struct igbvf_adapter)); > ... > alloc_etherdev() -> alloc_etherdev_mq() -> alloc_etherdev_mqs() -> > alloc_netdev_mqs() ... > | p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > 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> > --- Thanks Justin for these patches, please make sure you mark the subject line as per the netdev rules: [PATCH net-next v1] etc etc I'd also prefer they came in as part of one series with a good cover letter, at the very least for the Intel drivers, and you probably could combine any others (netdev) together up to the 15 patch limit. Please mention how you found these issues, via automated tool or via coccinelle script, manual grepping, etc? Thanks, Jesse
On 10/10/2023 2:20 PM, Jesse Brandeburg wrote: > On 10/10/2023 2:12 PM, 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. >> >> We expect netdev->name to be NUL-terminated based on its usage with >> `strlen` and format strings: >> | if (strlen(netdev->name) < (IFNAMSIZ - 5)) { >> | sprintf(adapter->tx_ring->name, "%s-tx-0", netdev->name); >> >> Moreover, we do not need NUL-padding as netdev is already >> zero-allocated: >> | netdev = alloc_etherdev(sizeof(struct igbvf_adapter)); >> ... >> alloc_etherdev() -> alloc_etherdev_mq() -> alloc_etherdev_mqs() -> >> alloc_netdev_mqs() ... >> | p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL); >> >> Considering the above, a suitable replacement is `strscpy` [2] due to >> the fact that it guarantees NUL-termination on the destination buffer >> without unnecessarily NUL-padding. >> >> 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> >> --- > > Thanks Justin for these patches, please make sure you mark the subject > line as per the netdev rules: > [PATCH net-next v1] etc etc > > I'd also prefer they came in as part of one series with a good cover > letter, at the very least for the Intel drivers, and you probably could > combine any others (netdev) together up to the 15 patch limit. > > Please mention how you found these issues, via automated tool or via > coccinelle script, manual grepping, etc? > > Thanks, > Jesse netdev rules: https://docs.kernel.org/process/maintainer-netdev.html Also, please see my related series, that will probably conflict with your changes but the two are making different changes. https://lore.kernel.org/netdev/20231003183603.3887546-1-jesse.brandeburg@intel.com/
On Tue, Oct 10, 2023 at 2:20 PM Jesse Brandeburg <jesse.brandeburg@intel.com> wrote: > > On 10/10/2023 2:12 PM, 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. > > > > We expect netdev->name to be NUL-terminated based on its usage with > > `strlen` and format strings: > > | if (strlen(netdev->name) < (IFNAMSIZ - 5)) { > > | sprintf(adapter->tx_ring->name, "%s-tx-0", netdev->name); > > > > Moreover, we do not need NUL-padding as netdev is already > > zero-allocated: > > | netdev = alloc_etherdev(sizeof(struct igbvf_adapter)); > > ... > > alloc_etherdev() -> alloc_etherdev_mq() -> alloc_etherdev_mqs() -> > > alloc_netdev_mqs() ... > > | p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL); > > > > Considering the above, a suitable replacement is `strscpy` [2] due to > > the fact that it guarantees NUL-termination on the destination buffer > > without unnecessarily NUL-padding. > > > > 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> > > --- > > Thanks Justin for these patches, please make sure you mark the subject > line as per the netdev rules: > [PATCH net-next v1] etc etc Sure, I'll resend! > > I'd also prefer they came in as part of one series with a good cover > letter, at the very least for the Intel drivers, and you probably could > combine any others (netdev) together up to the 15 patch limit. Got it :) > > Please mention how you found these issues, via automated tool or via > coccinelle script, manual grepping, etc? rg "strncpy\(" > pain.txt > > Thanks, > Jesse >
On Tue, Oct 10, 2023 at 09:12:00PM +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. > > We expect netdev->name to be NUL-terminated based on its usage with > `strlen` and format strings: > | if (strlen(netdev->name) < (IFNAMSIZ - 5)) { > | sprintf(adapter->tx_ring->name, "%s-tx-0", netdev->name); > > Moreover, we do not need NUL-padding as netdev is already > zero-allocated: > | netdev = alloc_etherdev(sizeof(struct igbvf_adapter)); > ... > alloc_etherdev() -> alloc_etherdev_mq() -> alloc_etherdev_mqs() -> > alloc_netdev_mqs() ... > | p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > 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/ethernet/intel/igbvf/netdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c > index 7ff2752dd763..fd712585af27 100644 > --- a/drivers/net/ethernet/intel/igbvf/netdev.c > +++ b/drivers/net/ethernet/intel/igbvf/netdev.c > @@ -2785,7 +2785,7 @@ static int igbvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > > igbvf_set_ethtool_ops(netdev); > netdev->watchdog_timeo = 5 * HZ; > - strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1); > + strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name)); > > adapter->bd_number = cards_found++; > > > --- > base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 > change-id: 20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-aea454a18a2d > > Best regards, > -- > Justin Stitt <justinstitt@google.com> > Hi, this patch was bundled up with some others. It has a new home: https://lore.kernel.org/all/20231010-netdev-replace-strncpy-resend-as-series-v1-0-caf9f0f2f021@google.com/
On Tue, 10 Oct 2023 14:41:10 -0700 Justin Stitt wrote: > > Thanks Justin for these patches, please make sure you mark the subject > > line as per the netdev rules: > > [PATCH net-next v1] etc etc > > Sure, I'll resend! Please do read the netdev rules Jesse pointed you at. Maybe it's the combined flow of strncpy and __counted_by patches but managing the state of the "hardening" patches is getting a bit tedious :( Please group them into reasonable series. Do not repost withing 24h. Do not have more than 15 patches for networking pending at any given time. That's basically the gist of our "good citizen" rules.
On Tue, 10 Oct 2023 17:47:31 -0700 Jakub Kicinski wrote: > Please do read the netdev rules Jesse pointed you at. > Maybe it's the combined flow of strncpy and __counted_by patches > but managing the state of the "hardening" patches is getting > a bit tedious :( > > Please group them into reasonable series. Do not repost withing 24h. > Do not have more than 15 patches for networking pending at any given > time. That's basically the gist of our "good citizen" rules. FWIW you can see how many pending patches you have pending in netdev using this here link: https://patchwork.kernel.org/project/netdevbpf/list/?submitter=206354
diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c index 7ff2752dd763..fd712585af27 100644 --- a/drivers/net/ethernet/intel/igbvf/netdev.c +++ b/drivers/net/ethernet/intel/igbvf/netdev.c @@ -2785,7 +2785,7 @@ static int igbvf_probe(struct pci_dev *pdev, const struct pci_device_id *ent) igbvf_set_ethtool_ops(netdev); netdev->watchdog_timeo = 5 * HZ; - strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1); + strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name)); adapter->bd_number = cards_found++;
`strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect netdev->name to be NUL-terminated based on its usage with `strlen` and format strings: | if (strlen(netdev->name) < (IFNAMSIZ - 5)) { | sprintf(adapter->tx_ring->name, "%s-tx-0", netdev->name); Moreover, we do not need NUL-padding as netdev is already zero-allocated: | netdev = alloc_etherdev(sizeof(struct igbvf_adapter)); ... alloc_etherdev() -> alloc_etherdev_mq() -> alloc_etherdev_mqs() -> alloc_netdev_mqs() ... | p = kvzalloc(alloc_size, GFP_KERNEL_ACCOUNT | __GFP_RETRY_MAYFAIL); Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. 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/ethernet/intel/igbvf/netdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-aea454a18a2d Best regards, -- Justin Stitt <justinstitt@google.com>