`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
NUL-padding is not necessary as host_info is initialized to
`ena_dev->host_attr.host_info` which is ultimately zero-initialized via
alloc_etherdev_mq().
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/amazon/ena/ena_netdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index f955bde10cf9..3118a617c9b6 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -3276,8 +3276,8 @@ static void ena_config_host_info(struct ena_com_dev *ena_dev, struct pci_dev *pd
strscpy(host_info->kernel_ver_str, utsname()->version,
sizeof(host_info->kernel_ver_str) - 1);
host_info->os_dist = 0;
- strncpy(host_info->os_dist_str, utsname()->release,
- sizeof(host_info->os_dist_str) - 1);
+ strscpy(host_info->os_dist_str, utsname()->release,
+ sizeof(host_info->os_dist_str));
host_info->driver_version =
(DRV_MODULE_GEN_MAJOR) |
(DRV_MODULE_GEN_MINOR << ENA_ADMIN_HOST_INFO_MINOR_SHIFT) |
---
base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
change-id: 20231005-strncpy-drivers-net-ethernet-amazon-ena-ena_netdev-c-6c4804466aa7
Best regards,
--
Justin Stitt <[email protected]>
On Thu, Oct 05, 2023 at 12:56:08AM +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.
>
> NUL-padding is not necessary as host_info is initialized to
> `ena_dev->host_attr.host_info` which is ultimately zero-initialized via
> alloc_etherdev_mq().
>
> 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]>
Looks right to me. Length nicely adjusted. :)
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
> -----Original Message-----
> From: Justin Stitt <[email protected]>
> Sent: Thursday, October 5, 2023 3:56 AM
> To: Agroskin, Shay <[email protected]>; Kiyanovski, Arthur
> <[email protected]>; Arinzon, David <[email protected]>; Dagan,
> Noam <[email protected]>; Bshara, Saeed <[email protected]>; David
> S. Miller <[email protected]>; Eric Dumazet <[email protected]>;
> Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Justin Stitt <[email protected]>
> Subject: [EXTERNAL] [PATCH] net: ena: replace deprecated strncpy with strscpy
>
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
>
>
>
> `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as
> such we should prefer more robust and less ambiguous string interfaces.
>
> NUL-padding is not necessary as host_info is initialized to `ena_dev-
> >host_attr.host_info` which is ultimately zero-initialized via
> alloc_etherdev_mq().
>
> 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/amazon/ena/ena_netdev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index f955bde10cf9..3118a617c9b6 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -3276,8 +3276,8 @@ static void ena_config_host_info(struct
> ena_com_dev *ena_dev, struct pci_dev *pd
> strscpy(host_info->kernel_ver_str, utsname()->version,
> sizeof(host_info->kernel_ver_str) - 1);
> host_info->os_dist = 0;
> - strncpy(host_info->os_dist_str, utsname()->release,
> - sizeof(host_info->os_dist_str) - 1);
> + strscpy(host_info->os_dist_str, utsname()->release,
> + sizeof(host_info->os_dist_str));
> host_info->driver_version =
> (DRV_MODULE_GEN_MAJOR) |
> (DRV_MODULE_GEN_MINOR <<
> ENA_ADMIN_HOST_INFO_MINOR_SHIFT) |
>
> ---
> base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
> change-id: 20231005-strncpy-drivers-net-ethernet-amazon-ena-ena_netdev-c-
> 6c4804466aa7
>
> Best regards,
> --
> Justin Stitt <[email protected]>
>
Thanks for submitting this change.
The change looks good but the sentence "NUL-padding is not necessary as
host_info is initialized to `ena_dev->host_attr.host_info` which is ultimately
zero-initialized via alloc_etherdev_mq()." is inaccurate.
host_info allocation is done in ena_com_allocate_host_info() via
dma_alloc_coherent() and is not zero initialized by alloc_etherdev_mq().
I looked at both the documentation of dma_alloc_coherent() in
https://www.kernel.org/doc/Documentation/DMA-API.txt
as well as the code itself, and (maybe I'm wrong but) I didn't see 100%
guarantees the that the memory is zero-initialized.
However zero initialization of the destination doesn't matter in this case,
because strscpy() guarantees a NULL termination.
So please just remove this sentence from the commit message.
Thanks,
Arthur Kiyanovski
On Thu, Oct 05, 2023 at 10:25:08PM +0000, Kiyanovski, Arthur wrote:
> > -----Original Message-----
> > From: Justin Stitt <[email protected]>
> > Sent: Thursday, October 5, 2023 3:56 AM
> > To: Agroskin, Shay <[email protected]>; Kiyanovski, Arthur
> > <[email protected]>; Arinzon, David <[email protected]>; Dagan,
> > Noam <[email protected]>; Bshara, Saeed <[email protected]>; David
> > S. Miller <[email protected]>; Eric Dumazet <[email protected]>;
> > Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; Justin Stitt <[email protected]>
> > Subject: [EXTERNAL] [PATCH] net: ena: replace deprecated strncpy with strscpy
> >
> > CAUTION: This email originated from outside of the organization. Do not click
> > links or open attachments unless you can confirm the sender and know the
> > content is safe.
> >
> >
> >
> > `strncpy` is deprecated for use on NUL-terminated destination strings [1] and as
> > such we should prefer more robust and less ambiguous string interfaces.
> >
> > NUL-padding is not necessary as host_info is initialized to `ena_dev-
> > >host_attr.host_info` which is ultimately zero-initialized via
> > alloc_etherdev_mq().
> >
> > 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/amazon/ena/ena_netdev.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > index f955bde10cf9..3118a617c9b6 100644
> > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > @@ -3276,8 +3276,8 @@ static void ena_config_host_info(struct
> > ena_com_dev *ena_dev, struct pci_dev *pd
> > strscpy(host_info->kernel_ver_str, utsname()->version,
> > sizeof(host_info->kernel_ver_str) - 1);
> > host_info->os_dist = 0;
> > - strncpy(host_info->os_dist_str, utsname()->release,
> > - sizeof(host_info->os_dist_str) - 1);
> > + strscpy(host_info->os_dist_str, utsname()->release,
> > + sizeof(host_info->os_dist_str));
> > host_info->driver_version =
> > (DRV_MODULE_GEN_MAJOR) |
> > (DRV_MODULE_GEN_MINOR <<
> > ENA_ADMIN_HOST_INFO_MINOR_SHIFT) |
> >
> > ---
> > base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
> > change-id: 20231005-strncpy-drivers-net-ethernet-amazon-ena-ena_netdev-c-
> > 6c4804466aa7
> >
> > Best regards,
> > --
> > Justin Stitt <[email protected]>
> >
>
> Thanks for submitting this change.
>
> The change looks good but the sentence "NUL-padding is not necessary as
> host_info is initialized to `ena_dev->host_attr.host_info` which is ultimately
> zero-initialized via alloc_etherdev_mq()." is inaccurate.
>
> host_info allocation is done in ena_com_allocate_host_info() via
> dma_alloc_coherent() and is not zero initialized by alloc_etherdev_mq().
>
> I looked at both the documentation of dma_alloc_coherent() in
> https://www.kernel.org/doc/Documentation/DMA-API.txt
> as well as the code itself, and (maybe I'm wrong but) I didn't see 100%
> guarantees the that the memory is zero-initialized.
>
> However zero initialization of the destination doesn't matter in this case,
> because strscpy() guarantees a NULL termination.
If this is in DMA memory, should the string buffer be %NUL-padded? (Or
is it consumed strictly as a %NUL-terminated string?)
-Kees
--
Kees Cook
> -----Original Message-----
> From: Kees Cook <[email protected]>
> Sent: Friday, October 6, 2023 1:39 AM
> To: Kiyanovski, Arthur <[email protected]>
> Cc: Justin Stitt <[email protected]>; Agroskin, Shay
> <[email protected]>; Arinzon, David <[email protected]>; Dagan,
> Noam <[email protected]>; Bshara, Saeed <[email protected]>; David
> S. Miller <[email protected]>; Eric Dumazet <[email protected]>;
> Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: RE: [EXTERNAL] [PATCH] net: ena: replace deprecated strncpy with
> strscpy
>
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
>
>
>
> On Thu, Oct 05, 2023 at 10:25:08PM +0000, Kiyanovski, Arthur wrote:
> > > -----Original Message-----
> > > From: Justin Stitt <[email protected]>
> > > Sent: Thursday, October 5, 2023 3:56 AM
> > > To: Agroskin, Shay <[email protected]>; Kiyanovski, Arthur
> > > <[email protected]>; Arinzon, David <[email protected]>; Dagan,
> > > Noam <[email protected]>; Bshara, Saeed <[email protected]>;
> David
> > > S. Miller <[email protected]>; Eric Dumazet <[email protected]>;
> > > Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>
> > > Cc: [email protected]; [email protected]; linux-
> > > [email protected]; Justin Stitt <[email protected]>
> > > Subject: [EXTERNAL] [PATCH] net: ena: replace deprecated strncpy
> > > with strscpy
> > >
> > > CAUTION: This email originated from outside of the organization. Do
> > > not click links or open attachments unless you can confirm the
> > > sender and know the content is safe.
> > >
> > >
> > >
> > > `strncpy` is deprecated for use on NUL-terminated destination
> > > strings [1] and as such we should prefer more robust and less ambiguous
> string interfaces.
> > >
> > > NUL-padding is not necessary as host_info is initialized to
> > > `ena_dev-
> > > >host_attr.host_info` which is ultimately zero-initialized via
> > > alloc_etherdev_mq().
> > >
> > > 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#strnc
> > > py-on-
> > > nul-terminated-strings [1]
> > > Link:
> > > https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.ht
> > > ml
> > > [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/amazon/ena/ena_netdev.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > > b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > > index f955bde10cf9..3118a617c9b6 100644
> > > --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > > +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> > > @@ -3276,8 +3276,8 @@ static void ena_config_host_info(struct
> > > ena_com_dev *ena_dev, struct pci_dev *pd
> > > strscpy(host_info->kernel_ver_str, utsname()->version,
> > > sizeof(host_info->kernel_ver_str) - 1);
> > > host_info->os_dist = 0;
> > > - strncpy(host_info->os_dist_str, utsname()->release,
> > > - sizeof(host_info->os_dist_str) - 1);
> > > + strscpy(host_info->os_dist_str, utsname()->release,
> > > + sizeof(host_info->os_dist_str));
> > > host_info->driver_version =
> > > (DRV_MODULE_GEN_MAJOR) |
> > > (DRV_MODULE_GEN_MINOR <<
> > > ENA_ADMIN_HOST_INFO_MINOR_SHIFT) |
> > >
> > > ---
> > > base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2
> > > change-id:
> > > 20231005-strncpy-drivers-net-ethernet-amazon-ena-ena_netdev-c-
> > > 6c4804466aa7
> > >
> > > Best regards,
> > > --
> > > Justin Stitt <[email protected]>
> > >
> >
> > Thanks for submitting this change.
> >
> > The change looks good but the sentence "NUL-padding is not necessary
> > as host_info is initialized to `ena_dev->host_attr.host_info` which is
> > ultimately zero-initialized via alloc_etherdev_mq()." is inaccurate.
> >
> > host_info allocation is done in ena_com_allocate_host_info() via
> > dma_alloc_coherent() and is not zero initialized by alloc_etherdev_mq().
> >
> > I looked at both the documentation of dma_alloc_coherent() in
> > https://www.kernel.org/doc/Documentation/DMA-API.txt
> > as well as the code itself, and (maybe I'm wrong but) I didn't see
> > 100% guarantees the that the memory is zero-initialized.
> >
> > However zero initialization of the destination doesn't matter in this
> > case, because strscpy() guarantees a NULL termination.
>
> If this is in DMA memory, should the string buffer be %NUL-padded? (Or is it
> consumed strictly as a %NUL-terminated string?)
>
> -Kees
>
> --
> Kees Cook
No need for NULL-padding, It is consumed strictly as a NULL-terminated string
Thanks,
Arthur Kiyanovski
On Thu, 05 Oct 2023 00:56:08 +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.
>
> NUL-padding is not necessary as host_info is initialized to
> `ena_dev->host_attr.host_info` which is ultimately zero-initialized via
> alloc_etherdev_mq().
>
> [...]
Applied to for-next/hardening, thanks!
[1/1] net: ena: replace deprecated strncpy with strscpy
https://git.kernel.org/kees/c/111f5a435d33
Take care,
--
Kees Cook
On Thu, 30 Nov 2023 13:59:48 -0800 Kees Cook wrote:
> [1/1] net: ena: replace deprecated strncpy with strscpy
> https://git.kernel.org/kees/c/111f5a435d33
Again, please drop, Arthur requested for the commit message
to be changed.
On Thu, Nov 30, 2023 at 10:41:34PM -0800, Jakub Kicinski wrote:
> On Thu, 30 Nov 2023 13:59:48 -0800 Kees Cook wrote:
> > [1/1] net: ena: replace deprecated strncpy with strscpy
> > https://git.kernel.org/kees/c/111f5a435d33
>
> Again, please drop, Arthur requested for the commit message
> to be changed.
Dropped, though I did change the commit message in the pulled commit.
Justin, can you send a v2 with the commit change? Then it can go through
regular netdev machinery?
--
Kees Cook