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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4a52a2ae-8abf-30e2-5c2a-d57280cb6028@linux-m68k.org>
Date: Sat, 24 Feb 2024 10:44:12 +1100 (AEDT)
From: Finn Thain <fthain@...ux-m68k.org>
To: Justin Stitt <justinstitt@...gle.com>
cc: Sathya Prakash Veerichetty <sathya.prakash@...adcom.com>,
     Kashyap Desai <kashyap.desai@...adcom.com>,
     Sumit Saxena <sumit.saxena@...adcom.com>,
     Sreekanth Reddy <sreekanth.reddy@...adcom.com>,
     "James E.J. Bottomley" <jejb@...ux.ibm.com>,
     "Martin K. Petersen" <martin.petersen@...cle.com>,
     Suganath Prabu Subramani <suganath-prabu.subramani@...adcom.com>,
     Ariel Elior <aelior@...vell.com>,
 Manish Chopra <manishc@...vell.com>,
     "David S. Miller" <davem@...emloft.net>,
     Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
     Paolo Abeni <pabeni@...hat.com>,
 Saurav Kashyap <skashyap@...vell.com>,
     Javed Hasan <jhasan@...vell.com>,
 GR-QLogic-Storage-Upstream@...vell.com,
     Nilesh Javali <njavali@...vell.com>,
     Manish Rangankar <mrangankar@...vell.com>,
     Don Brace <don.brace@...rochip.com>,
 mpi3mr-linuxdrv.pdl@...adcom.com,     linux-scsi@...r.kernel.org,
 linux-hardening@...r.kernel.org,     linux-kernel@...r.kernel.org,
 Kees Cook <keescook@...omium.org>,     MPT-FusionLinux.pdl@...adcom.com,
 netdev@...r.kernel.org,     storagedev@...rochip.com
Subject: Re: [PATCH 7/7] scsi: wd33c93: replace deprecated strncpy with
 strscpy


On Fri, 23 Feb 2024, Justin Stitt wrote:

> @p1 is assigned to @setup_buffer and then we manually assign a NUL-byte
> at the first index. This renders the following strlen() call useless.
> Moreover, we don't need to reassign p1 to setup_buffer for any reason --
> neither do we need to manually set a NUL-byte at the end. strscpy()
> resolves all this code making it easier to read.
> 
> Even considering the path where @str is falsey, the manual NUL-byte
> assignment is useless 

And yet your patch would only remove one of those assignments...

> as setup_buffer is declared with static storage
> duration in the top-level scope which should NUL-initialize the whole
> buffer.
> 

So, in order to review this patch, to try to avoid regressions, I would 
have to check your assumption that setup_buffer cannot change after being 
statically initialized. (The author of this code apparently was not 
willing to make that assumption.) It seems that patch review would require 
exhaustively searching for functions using the buffer, and examining the 
call graphs involving those functions. Is it really worth the effort?

> Signed-off-by: Justin Stitt <justinstitt@...gle.com>
> ---
>  drivers/scsi/wd33c93.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/wd33c93.c b/drivers/scsi/wd33c93.c
> index e4fafc77bd20..a44b60c9004a 100644
> --- a/drivers/scsi/wd33c93.c
> +++ b/drivers/scsi/wd33c93.c
> @@ -1721,9 +1721,7 @@ wd33c93_setup(char *str)
>  	p1 = setup_buffer;
>  	*p1 = '\0';
>  	if (str)
> -		strncpy(p1, str, SETUP_BUFFER_SIZE - strlen(setup_buffer));
> -	setup_buffer[SETUP_BUFFER_SIZE - 1] = '\0';
> -	p1 = setup_buffer;
> +		strscpy(p1, str, SETUP_BUFFER_SIZE);
>  	i = 0;
>  	while (*p1 && (i < MAX_SETUP_ARGS)) {
>  		p2 = strchr(p1, ',');
> 
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ