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/[email protected]/
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 <[email protected]>
---
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 <[email protected]>
`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
The "...-1" pattern makes it evident that netdev->name is expected to be
NUL-terminated.
Meanwhile, it seems NUL-padding is not required due to alloc_etherdev
zero-allocating the buffer.
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.
This is in line with other uses of strscpy on netdev->name:
$ rg "strscpy\(netdev\->name.*pci.*"
drivers/net/ethernet/intel/e1000e/netdev.c
7455: strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name));
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
10839: strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name));
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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
---
drivers/net/ethernet/intel/e100.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index d3fdc290937f..01f0f12035ca 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2841,7 +2841,7 @@ static int e100_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
netdev->netdev_ops = &e100_netdev_ops;
netdev->ethtool_ops = &e100_ethtool_ops;
netdev->watchdog_timeo = E100_WATCHDOG_PERIOD;
- strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
+ strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name));
nic = netdev_priv(netdev);
netif_napi_add_weight(netdev, &nic->napi, e100_poll, E100_NAPI_WEIGHT);
--
2.42.0.609.gbb76f46606-goog
`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
Moreover, `strncat` shouldn't really be used either as per
fortify-string.h:
* Do not use this function. While FORTIFY_SOURCE tries to avoid
* read and write overflows, this is only possible when the sizes
* of @p and @q are known to the compiler. Prefer building the
* string with formatting, via scnprintf() or similar.
Instead, use `scnprintf` with "%s%s" format string. This code is now
more readable and robust.
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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
---
drivers/net/ethernet/intel/i40e/i40e_ddp.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_ddp.c b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
index 0e72abd178ae..ec25e4be250f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ddp.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ddp.c
@@ -438,10 +438,9 @@ int i40e_ddp_flash(struct net_device *netdev, struct ethtool_flash *flash)
char profile_name[sizeof(I40E_DDP_PROFILE_PATH)
+ I40E_DDP_PROFILE_NAME_MAX];
- profile_name[sizeof(profile_name) - 1] = 0;
- strncpy(profile_name, I40E_DDP_PROFILE_PATH,
- sizeof(profile_name) - 1);
- strncat(profile_name, flash->data, I40E_DDP_PROFILE_NAME_MAX);
+ scnprintf(profile_name, sizeof(profile_name), "%s%s",
+ I40E_DDP_PROFILE_PATH, flash->data);
+
/* Load DDP recipe. */
status = request_firmware(&ddp_config, profile_name,
&netdev->dev);
--
2.42.0.609.gbb76f46606-goog
`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
Other implementations of .*get_drvinfo also use strscpy so this patch
brings fm10k_get_drvinfo in line as well:
igb/igb_ethtool.c +851
static void igb_get_drvinfo(struct net_device *netdev,
igbvf/ethtool.c
167:static void igbvf_get_drvinfo(struct net_device *netdev,
i40e/i40e_ethtool.c
1999:static void i40e_get_drvinfo(struct net_device *netdev,
e1000/e1000_ethtool.c
529:static void e1000_get_drvinfo(struct net_device *netdev,
ixgbevf/ethtool.c
211:static void ixgbevf_get_drvinfo(struct net_device *netdev,
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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
---
drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
index d53369e30040..13a05604dcc0 100644
--- a/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
+++ b/drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
@@ -448,10 +448,10 @@ static void fm10k_get_drvinfo(struct net_device *dev,
{
struct fm10k_intfc *interface = netdev_priv(dev);
- strncpy(info->driver, fm10k_driver_name,
- sizeof(info->driver) - 1);
- strncpy(info->bus_info, pci_name(interface->pdev),
- sizeof(info->bus_info) - 1);
+ strscpy(info->driver, fm10k_driver_name,
+ sizeof(info->driver));
+ strscpy(info->bus_info, pci_name(interface->pdev),
+ sizeof(info->bus_info));
}
static void fm10k_get_pauseparam(struct net_device *dev,
--
2.42.0.609.gbb76f46606-goog
`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 see that netdev->name is expected to be NUL-terminated based on its
usage with format strings:
| sprintf(q_vector->name, "%s-TxRx-%u", netdev->name,
| q_vector->rx.ring->queue_index);
Furthermore, NUL-padding is not required as netdev is already
zero-allocated:
| netdev = alloc_etherdev_mq(sizeof(struct igb_adapter),
| IGB_MAX_TX_QUEUES);
...
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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
---
drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 76b34cee1da3..9de103bd3ad3 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3264,7 +3264,7 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
igb_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));
netdev->mem_start = pci_resource_start(pdev, 0);
netdev->mem_end = pci_resource_end(pdev, 0);
--
2.42.0.609.gbb76f46606-goog
`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 can see that netdev->name is expected to be NUL-terminated based on
it's usage with format strings:
| pr_info("%s NIC Link is Down\n",
| netdev->name);
A suitable replacement is `strscpy` [2] due to the fact that it
guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
This is in line with other uses of strscpy on netdev->name:
$ rg "strscpy\(netdev\->name.*pci.*"
drivers/net/ethernet/intel/e1000e/netdev.c
7455: strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name));
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
10839: strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name));
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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index da6e303ad99b..1d1e93686af2 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1014,7 +1014,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
netdev->watchdog_timeo = 5 * HZ;
netif_napi_add(netdev, &adapter->napi, e1000_clean);
- strncpy(netdev->name, pci_name(pdev), sizeof(netdev->name) - 1);
+ strscpy(netdev->name, pci_name(pdev), sizeof(netdev->name));
adapter->bd_number = cards_found;
--
2.42.0.609.gbb76f46606-goog
`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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
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++;
--
2.42.0.609.gbb76f46606-goog
`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 use with format
strings:
| if (q_vector->rx.ring && q_vector->tx.ring)
| sprintf(q_vector->name, "%s-TxRx-%u", netdev->name,
Furthermore, we do not need NUL-padding as netdev is already
zero-allocated:
| netdev = alloc_etherdev_mq(sizeof(struct igc_adapter),
| IGC_MAX_TX_QUEUES);
...
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: [email protected]
Signed-off-by: Justin Stitt <[email protected]>
---
Note: build-tested only.
---
drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 98de34d0ce07..e9bb403bbacf 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -6935,7 +6935,7 @@ static int igc_probe(struct pci_dev *pdev,
*/
igc_get_hw_control(adapter);
- strncpy(netdev->name, "eth%d", IFNAMSIZ);
+ strscpy(netdev->name, "eth%d", sizeof(netdev->name));
err = register_netdev(netdev);
if (err)
goto err_register;
--
2.42.0.609.gbb76f46606-goog
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/[email protected]/
>
> 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 <[email protected]>
Thanks Justin for fixing all these!
For the series:
Reviewed-by: Jesse Brandeburg <[email protected]>
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
<[email protected]> 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/[email protected]/
> >
> > 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 <[email protected]>
>
> Thanks Justin for fixing all these!
>
> For the series:
> Reviewed-by: Jesse Brandeburg <[email protected]>
>
> 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
> <[email protected]> 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/[email protected]/
> > >
> > > 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 <[email protected]>
> >
> > Thanks Justin for fixing all these!
> >
> > For the series:
> > Reviewed-by: Jesse Brandeburg <[email protected]>
> >
> > 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. :)
--
Kees Cook
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/[email protected]/
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
-Kees
--
Kees Cook
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
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of Justin Stitt
> Sent: Wednesday, October 11, 2023 3:57 AM
> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>
> Cc: [email protected]; Justin Stitt <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: [Intel-wired-lan] [PATCH v1 net-next 7/7] igc: replace deprecated strncpy with strscpy
>
> `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 use with format
> strings:
> | if (q_vector->rx.ring && q_vector->tx.ring)
> | sprintf(q_vector->name, "%s-TxRx-%u", netdev->name,
>
> Furthermore, we do not need NUL-padding as netdev is already
> zero-allocated:
> | netdev = alloc_etherdev_mq(sizeof(struct igc_adapter),
> | IGC_MAX_TX_QUEUES);
> ...
> 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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
>
> ---
> Note: build-tested only.
> ---
> drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of Justin Stitt
> Sent: Wednesday, October 11, 2023 3:57 AM
> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>
> Cc: [email protected]; Justin Stitt <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: [Intel-wired-lan] [PATCH v1 net-next 6/7] igbvf: replace deprecated strncpy with strscpy
>
> `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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
>
> ---
> Note: build-tested only.
> ---
> drivers/net/ethernet/intel/igbvf/netdev.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of Justin Stitt
> Sent: Wednesday, October 11, 2023 3:57 AM
> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>
> Cc: [email protected]; Justin Stitt <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: [Intel-wired-lan] [PATCH v1 net-next 5/7] igb: replace deprecated strncpy with strscpy
>
> `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 see that netdev->name is expected to be NUL-terminated based on its
> usage with format strings:
> | sprintf(q_vector->name, "%s-TxRx-%u", netdev->name,
> | q_vector->rx.ring->queue_index);
>
> Furthermore, NUL-padding is not required as netdev is already
> zero-allocated:
> | netdev = alloc_etherdev_mq(sizeof(struct igb_adapter),
> | IGB_MAX_TX_QUEUES);
> ...
> 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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
>
> ---
> Note: build-tested only.
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)
> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of Justin Stitt
> Sent: Wednesday, October 11, 2023 3:57 AM
> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L <[email protected]>; David S. Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>
> Cc: [email protected]; Justin Stitt <[email protected]>; [email protected]; [email protected]; [email protected]
> Subject: [Intel-wired-lan] [PATCH v1 net-next 4/7] i40e: use scnprintf over strncpy+strncat
>
> `strncpy` is deprecated for use on NUL-terminated destination strings
> [1] and as such we should prefer more robust and less ambiguous string
> interfaces.
>
> Moreover, `strncat` shouldn't really be used either as per
> fortify-string.h:
> * Do not use this function. While FORTIFY_SOURCE tries to avoid
> * read and write overflows, this is only possible when the sizes
> * of @p and @q are known to the compiler. Prefer building the
> * string with formatting, via scnprintf() or similar.
>
> Instead, use `scnprintf` with "%s%s" format string. This code is now
> more readable and robust.
>
> 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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
>
> ---
> Note: build-tested only.
> ---
> drivers/net/ethernet/intel/i40e/i40e_ddp.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
Tested-by: Pucha Himasekhar Reddy <[email protected]> (A Contingent worker at Intel)