linux-kernel - Re: [PATCH v3 1/2] Staging: rts5208: Replace 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <191cf6e2-6a46-4813-a0c4-c86bcf9bd0b2@kadam.mountain>
Date:   Tue, 31 Oct 2023 07:13:12 +0300
From:   Dan Carpenter <dan.carpenter@...aro.org>
To:     Nancy Nyambura <nicymimz@...il.com>
Cc:     nicydaniels@...il.com, outreachy@...ts.linux.dev,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] Staging: rts5208: Replace strncpy() with strscpy()

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