lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <5f8d24d16fba49bfb57fd4b6678ac27d@amazon.com> Date: Fri, 6 Oct 2023 11:02:22 +0000 From: "Kiyanovski, Arthur" <akiyano@...zon.com> To: Kees Cook <keescook@...omium.org> CC: Justin Stitt <justinstitt@...gle.com>, "Agroskin, Shay" <shayagr@...zon.com>, "Arinzon, David" <darinzon@...zon.com>, "Dagan, Noam" <ndagan@...zon.com>, "Bshara, Saeed" <saeedb@...zon.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org> Subject: RE: [PATCH] net: ena: replace deprecated strncpy with strscpy > -----Original Message----- > From: Kees Cook <keescook@...omium.org> > Sent: Friday, October 6, 2023 1:39 AM > To: Kiyanovski, Arthur <akiyano@...zon.com> > Cc: Justin Stitt <justinstitt@...gle.com>; Agroskin, Shay > <shayagr@...zon.com>; Arinzon, David <darinzon@...zon.com>; Dagan, > Noam <ndagan@...zon.com>; Bshara, Saeed <saeedb@...zon.com>; David > S. Miller <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; > Jakub Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com>; > netdev@...r.kernel.org; linux-kernel@...r.kernel.org; linux- > hardening@...r.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@...gle.com> > > > Sent: Thursday, October 5, 2023 3:56 AM > > > To: Agroskin, Shay <shayagr@...zon.com>; Kiyanovski, Arthur > > > <akiyano@...zon.com>; Arinzon, David <darinzon@...zon.com>; Dagan, > > > Noam <ndagan@...zon.com>; Bshara, Saeed <saeedb@...zon.com>; > David > > > S. Miller <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; > > > Jakub Kicinski <kuba@...nel.org>; Paolo Abeni <pabeni@...hat.com> > > > Cc: netdev@...r.kernel.org; linux-kernel@...r.kernel.org; linux- > > > hardening@...r.kernel.org; Justin Stitt <justinstitt@...gle.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@...r.kernel.org > > > Signed-off-by: Justin Stitt <justinstitt@...gle.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@...gle.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
Powered by blists - more mailing lists