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

[Kiyanovski, Arthur]

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