[PATCH] net: ena: replace deprecated strncpy with strscpy

[PATCH] net: ena: replace deprecated strncpy with strscpy

[Justin Stitt]

`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: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
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 <justinstitt@google.com>

Re: [PATCH] net: ena: replace deprecated strncpy with strscpy

[Kees Cook]

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: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Looks right to me. Length nicely adjusted. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

RE: [PATCH] net: ena: replace deprecated strncpy with strscpy

[Kiyanovski, Arthur]

> -----Original Message-----
> From: Justin Stitt <justinstitt@google.com>
> Sent: Thursday, October 5, 2023 3:56 AM
> To: Agroskin, Shay <shayagr@amazon.com>; Kiyanovski, Arthur
> <akiyano@amazon.com>; Arinzon, David <darinzon@amazon.com>; Dagan,
> Noam <ndagan@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; David
> S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> hardening@vger.kernel.org; Justin Stitt <justinstitt@google.com>
> 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: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> 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 <justinstitt@google.com>
> 

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

Re: [PATCH] net: ena: replace deprecated strncpy with strscpy

[Kees Cook]

On Thu, Oct 05, 2023 at 10:25:08PM +0000, Kiyanovski, Arthur wrote:
> > -----Original Message-----
> > From: Justin Stitt <justinstitt@google.com>
> > Sent: Thursday, October 5, 2023 3:56 AM
> > To: Agroskin, Shay <shayagr@amazon.com>; Kiyanovski, Arthur
> > <akiyano@amazon.com>; Arinzon, David <darinzon@amazon.com>; Dagan,
> > Noam <ndagan@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; David
> > S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> > Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > hardening@vger.kernel.org; Justin Stitt <justinstitt@google.com>
> > 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: linux-hardening@vger.kernel.org
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > ---
> > 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 <justinstitt@google.com>
> > 
> 
> 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

RE: [PATCH] net: ena: replace deprecated strncpy with strscpy

[Kiyanovski, Arthur]

> -----Original Message-----
> From: Kees Cook <keescook@chromium.org>
> Sent: Friday, October 6, 2023 1:39 AM
> To: Kiyanovski, Arthur <akiyano@amazon.com>
> Cc: Justin Stitt <justinstitt@google.com>; Agroskin, Shay
> <shayagr@amazon.com>; Arinzon, David <darinzon@amazon.com>; Dagan,
> Noam <ndagan@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; David
> S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> hardening@vger.kernel.org
> 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 <justinstitt@google.com>
> > > Sent: Thursday, October 5, 2023 3:56 AM
> > > To: Agroskin, Shay <shayagr@amazon.com>; Kiyanovski, Arthur
> > > <akiyano@amazon.com>; Arinzon, David <darinzon@amazon.com>; Dagan,
> > > Noam <ndagan@amazon.com>; Bshara, Saeed <saeedb@amazon.com>;
> David
> > > S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> > > Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>
> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> > > hardening@vger.kernel.org; Justin Stitt <justinstitt@google.com>
> > > 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: linux-hardening@vger.kernel.org
> > > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > > ---
> > > 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 <justinstitt@google.com>
> > >
> >
> > 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

Re: [PATCH] net: ena: replace deprecated strncpy with strscpy

[Kees Cook]

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

Re: [PATCH] net: ena: replace deprecated strncpy with strscpy

[Jakub Kicinski]

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.

Re: [PATCH] net: ena: replace deprecated strncpy with strscpy

[Kees Cook]

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