Received-SPF: pass (google.com: domain of linux-kernel+bounces-79331-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 23 Feb 2024 16:01:18 -0800
From: Kees Cook <keescook@chromium.org>
To: Finn Thain <fthain@linux-m68k.org>
Cc: Justin Stitt <justinstitt@google.com>,
	Sathya Prakash Veerichetty <sathya.prakash@broadcom.com>,
	Kashyap Desai <kashyap.desai@broadcom.com>,
	Sumit Saxena <sumit.saxena@broadcom.com>,
	Sreekanth Reddy <sreekanth.reddy@broadcom.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>,
	Ariel Elior <aelior@marvell.com>,
	Manish Chopra <manishc@marvell.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Saurav Kashyap <skashyap@marvell.com>,
	Javed Hasan <jhasan@marvell.com>,
	GR-QLogic-Storage-Upstream@marvell.com,
	Nilesh Javali <njavali@marvell.com>,
	Manish Rangankar <mrangankar@marvell.com>,
	Don Brace <don.brace@microchip.com>,
	mpi3mr-linuxdrv.pdl@broadcom.com, linux-scsi@vger.kernel.org,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	MPT-FusionLinux.pdl@broadcom.com, netdev@vger.kernel.org,
	storagedev@microchip.com
Subject: Re: [PATCH 7/7] scsi: wd33c93: replace deprecated strncpy with
 strscpy
Message-ID: <202402231556.7DBA6E1@keescook>
References: <20240223-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v1-0-9cd3882f0700@google.com>
 <20240223-strncpy-drivers-scsi-mpi3mr-mpi3mr_fw-c-v1-7-9cd3882f0700@google.com>
 <4a52a2ae-8abf-30e2-5c2a-d57280cb6028@linux-m68k.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4a52a2ae-8abf-30e2-5c2a-d57280cb6028@linux-m68k.org>

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@google.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@chromium.org>

-- 
Kees Cook