[PATCH v3 1/2] Staging: rts5208: Replace strncpy() with strscpy()

[PATCH v3 1/2] Staging: rts5208: Replace strncpy() with strscpy()

[Nancy Nyambura]

Warning found by checkpath.pl
I replaced strncpy with strscpy because strscpy is suitable when the destination buffer is NUL-terminated, which is often the case when copying strings. Strscpy ensures that the destination buffer is properly NUL-terminated without padding. In the code, the objective is to copy a string (inquiry_string) to the buf buffer, and it's likely that the buf buffer is NUL-terminated since it is handling a string. Strscpy_pad is used when you have afixed-size buffer, and you want to copy a string into it while ensuring the remaining space is padded with a specific character (like '\0') hence not appropriate for this context.

Signed-off-by: Nancy Nyambura <nicymimz@gmail.com>
---
Changes in v2:
  - Explain why strscpy and not strscpy_pad

 drivers/staging/rts5208/rtsx_scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/rts5208/rtsx_scsi.c b/drivers/staging/rts5208/rtsx_scsi.c
index 08bd768ad34d..52324b8ebbc7 100644
--- a/drivers/staging/rts5208/rtsx_scsi.c
+++ b/drivers/staging/rts5208/rtsx_scsi.c
@@ -523,7 +523,7 @@ static int inquiry(struct scsi_cmnd *srb, struct rtsx_chip *chip)
 
 	if (sendbytes > 8) {
 		memcpy(buf, inquiry_buf, 8);
-		strncpy(buf + 8, inquiry_string, sendbytes - 8);
+		strscpy(buf + 8, inquiry_string, sendbytes - 8);
 		if (pro_formatter_flag) {
 			/* Additional Length */
 			buf[4] = 0x33;
-- 
2.40.1

Re: [PATCH v3 1/2] Staging: rts5208: Replace strncpy() with strscpy()

[Dan Carpenter]

On Mon, Oct 30, 2023 at 05:27:47PM +0300, Nancy Nyambura wrote:
> Warning found by checkpath.pl
> I replaced strncpy with strscpy because strscpy is suitable when the
> destination buffer is NUL-terminated, which is often the case when
> copying strings. Strscpy ensures that the destination buffer is
> properly NUL-terminated without padding.

The same is basically true for strncpy()...  In olden days strncpy()
was the only "safe" function we had so we used it exactly how we use
strscpy() today except we manually added a NUL terminator to the end.

	new = kzalloc(len + 1, GFP_KERNEL);
	strncpy(new, old, len);

> In the code, the objective is to copy a string (inquiry_string) to the
> buf buffer, and it's likely that the buf buffer is NUL-terminated
> since it is handling a string. Strscpy_pad is used when you have
> afixed-size buffer, and you want to copy a string into it while
> ensuring the remaining space is padded with a specific character
> (like '\0') hence not appropriate for this context.
> 

You need to run your patches through checkpatch.

"it's likely that the buf buffer is NUL-terminated since it is handling
a string."  That's not analysis.  That's just guessing.  Take what time
you need and do the analysis.

"Strscpy_pad is used when you have afixed-size buffer, and you want to
copy a string into it while ensuring the remaining space is padded with
a specific character (like '\0') hence not appropriate for this context."

It's not "like '\0'" it's specifically '\0'...  This explains what
strscpy_pad() does.  We all know what it does.  No need to explain that.
However this doesn't explain *why* it's not appropriate.

1) Look at buf.  How big is it?  What data is stored in buf before we do
   the strncpy().
2) Look at inquiry_string.  Where does it come from?  Is it NUL
   terminated?  How long is it?
3) Look at sendbytes.  What length is it?  Is it longer than the size of
   buf + 8?  Is it longer than the size of inquiry_string?
4) What do we do with buf after the strncpy()?  Does the surrounding
   code assume that it is NUL terminated?  Do we copy the code to the
   user?

Once you know the answers to all these questions, then figure out which
of the questions matter in this context.  Re-write the commit message
with the relevant information.

I'm going to give you a hint here.

	rtsx_stor_set_xfer_buf(buf, scsi_bufflen(srb), srb);

This function copies "buf" to the user.

regards,
dan carpenter

Re: [PATCH v3 1/2] Staging: rts5208: Replace strncpy() with strscpy()

[Dan Carpenter]

I have updated my blog with more comments and examples.

https://staticthinking.wordpress.com/2023/10/30/strcpy-strncpy-strlcpy-and-strscpy/

regards,
dan carpenter