linux-hardening - Re: [PATCH 7/7] scsi: wd33c93: replace deprecated strncpy with strscpy

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 23 Feb 2024 16:01:18 -0800
From: Kees Cook <keescook@...omium.org>
To: Finn Thain <fthain@...ux-m68k.org>
Cc: Justin Stitt <justinstitt@...gle.com>,
	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,
	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 Sat, Feb 24, 2024 at 10:44:12AM +1100, Finn Thain wrote:
> 
> 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...

The first is needed in case it is called again.

> 
> > 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?

It seems to be run for each device? Regardless, I think leaving the
initial "*p1 = '\0';" solves this. (Though I fear for parallel
initializations, but that was already buggy: this code is from pre-git
history...)

> 
> > 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, ',');
> > 
> > 

I think this conversion looks right.

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

-- 
Kees Cook