`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++;
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-aea454a18a2d
Best regards,
--
Justin Stitt <[email protected]>
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: [email protected]
> Signed-off-by: Justin Stitt <[email protected]>
> ---
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: [email protected]
>> Signed-off-by: Justin Stitt <[email protected]>
>> ---
>
> 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/[email protected]/
On Tue, Oct 10, 2023 at 2:20 PM Jesse Brandeburg
<[email protected]> 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: [email protected]
> > Signed-off-by: Justin Stitt <[email protected]>
> > ---
>
> 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: [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++;
>
>
> ---
> base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
> change-id: 20231010-strncpy-drivers-net-ethernet-intel-igbvf-netdev-c-aea454a18a2d
>
> Best regards,
> --
> Justin Stitt <[email protected]>
>
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