[Linux-kernel-mentees] [PATCH] checkpatch: add a new check for strcpy/strlcpy uses

[Linux-kernel-mentees] [PATCH] checkpatch: add a new check for strcpy/strlcpy uses

[Dwaipayan Ray]

strcpy() performs no bounds checking on the destination buffer.
This could result in linear overflows beyond the end of the buffer.

strlcpy() reads the entire source buffer first. This read
may exceed the destination size limit. This can be both inefficient
and lead to linear read overflows.

The safe replacement to both of these is to use strscpy() instead.
Add a new checkpatch warning which alerts the user on finding usage of
strcpy() or strlcpy().

Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d6a4d25b0972..0003fd9de62c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6604,6 +6604,13 @@ sub process {
 			}
 		}
 
+# Check for strcpy/strlcpy uses
+		if (defined($stat) &&
+		    $stat =~ /^\+(?:.*?)\b(str[l]?cpy)\s*\(/) {
+			WARN("PREFER_STRSCPY",
+			     "Prefer strscpy() over $1()\n" . "$here\n$stat\n");
+		}
+
 # Check for memcpy(foo, bar, ETH_ALEN) that could be ether_addr_copy(foo, bar)
 #		if ($perl_version_ok &&
 #		    defined $stat &&
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add a new check for strcpy/strlcpy uses

[Joe Perches]

On Tue, 2021-01-05 at 13:53 +0530, Dwaipayan Ray wrote:
> strcpy() performs no bounds checking on the destination buffer.
> This could result in linear overflows beyond the end of the buffer.
> 
> strlcpy() reads the entire source buffer first. This read
> may exceed the destination size limit. This can be both inefficient
> and lead to linear read overflows.
> 
> The safe replacement to both of these is to use strscpy() instead.
> Add a new checkpatch warning which alerts the user on finding usage of
> strcpy() or strlcpy().

I do not believe that strscpy is preferred over strcpy.

When the size of the output buffer is known to be larger
than the input, strcpy is faster.

There are about 2k uses of strcpy.
Is there a use where strcpy use actually matters?
I don't know offhand...

But I believe compilers do not optimize away the uses of strscpy
to a simple memcpy like they do for strcpy with a const from

	strcpy(foo, "bar");

And lastly there is a existing strlcpy test in checkpatch.

commit 5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add a new check for strcpy/strlcpy uses

[Dwaipayan Ray]

On Tue, Jan 5, 2021 at 2:14 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2021-01-05 at 13:53 +0530, Dwaipayan Ray wrote:
> > strcpy() performs no bounds checking on the destination buffer.
> > This could result in linear overflows beyond the end of the buffer.
> >
> > strlcpy() reads the entire source buffer first. This read
> > may exceed the destination size limit. This can be both inefficient
> > and lead to linear read overflows.
> >
> > The safe replacement to both of these is to use strscpy() instead.
> > Add a new checkpatch warning which alerts the user on finding usage of
> > strcpy() or strlcpy().
>
> I do not believe that strscpy is preferred over strcpy.
>
> When the size of the output buffer is known to be larger
> than the input, strcpy is faster.
>
> There are about 2k uses of strcpy.
> Is there a use where strcpy use actually matters?
> I don't know offhand...
>
> But I believe compilers do not optimize away the uses of strscpy
> to a simple memcpy like they do for strcpy with a const from
>
>         strcpy(foo, "bar");
>

Yes the optimization here definitely helps. So in case the programmer
knows that the destination buffer is always larger, then strcpy() should be
preferred? I think the documentation might have been too strict about
strcpy() uses here:

Documentation/process/deprecated.rst:
"strcpy() performs no bounds checking on the destination buffer. This
could result in linear overflows beyond the end of the buffer, leading to
all kinds of misbehaviors. While `CONFIG_FORTIFY_SOURCE=y` and various
compiler flags help reduce the risk of using this function, there is
no good reason to add new uses of this function. The safe replacement
is strscpy(),..."


> And lastly there is a existing strlcpy test in checkpatch.
>
> commit 5dbdb2d87c29 ("checkpatch: prefer strscpy to strlcpy")
>
I will drop this patch. Thanks for your view.

Thank you,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

[Linux-kernel-mentees] deprecated.rst: deprecated strcpy ? (was: [PATCH] checkpatch: add a new check for strcpy/strlcpy uses)

[Joe Perches]

On Tue, 2021-01-05 at 14:29 +0530, Dwaipayan Ray wrote:
> On Tue, Jan 5, 2021 at 2:14 PM Joe Perches <joe@perches.com> wrote:
> > 
> > On Tue, 2021-01-05 at 13:53 +0530, Dwaipayan Ray wrote:
> > > strcpy() performs no bounds checking on the destination buffer.
> > > This could result in linear overflows beyond the end of the buffer.
> > > 
> > > strlcpy() reads the entire source buffer first. This read
> > > may exceed the destination size limit. This can be both inefficient
> > > and lead to linear read overflows.
> > > 
> > > The safe replacement to both of these is to use strscpy() instead.
> > > Add a new checkpatch warning which alerts the user on finding usage of
> > > strcpy() or strlcpy().
> > 
> > I do not believe that strscpy is preferred over strcpy.
> > 
> > When the size of the output buffer is known to be larger
> > than the input, strcpy is faster.
> > 
> > There are about 2k uses of strcpy.
> > Is there a use where strcpy use actually matters?
> > I don't know offhand...
> > 
> > But I believe compilers do not optimize away the uses of strscpy
> > to a simple memcpy like they do for strcpy with a const from
> > 
> >         strcpy(foo, "bar");
> > 
> 
> Yes the optimization here definitely helps. So in case the programmer
> knows that the destination buffer is always larger, then strcpy() should be
> preferred? I think the documentation might have been too strict about
> strcpy() uses here:
> 
> Documentation/process/deprecated.rst:
> "strcpy() performs no bounds checking on the destination buffer. This
> could result in linear overflows beyond the end of the buffer, leading to
> all kinds of misbehaviors. While `CONFIG_FORTIFY_SOURCE=y` and various
> compiler flags help reduce the risk of using this function, there is
> no good reason to add new uses of this function. The safe replacement
> is strscpy(),..."

Kees/Jonathan:

Perhaps this text is overly restrictive.

There are ~2k uses of strcpy in the kernel.

About half of these are where the buffer length of foo is known and the
use is 'strcpy(foo, "bar")' so the compiler converts/optimizes away the
strcpy to memcpy and may not even put "bar" into the string table.

I believe strscpy uses do not have this optimization.

Is there a case where the runtime costs actually matters?
I expect so.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] deprecated.rst: deprecated strcpy ? (was: [PATCH] checkpatch: add a new check for strcpy/strlcpy uses)

[Kees Cook]

On Tue, Jan 05, 2021 at 01:28:18AM -0800, Joe Perches wrote:
> On Tue, 2021-01-05 at 14:29 +0530, Dwaipayan Ray wrote:
> > On Tue, Jan 5, 2021 at 2:14 PM Joe Perches <joe@perches.com> wrote:
> > > 
> > > On Tue, 2021-01-05 at 13:53 +0530, Dwaipayan Ray wrote:
> > > > strcpy() performs no bounds checking on the destination buffer.
> > > > This could result in linear overflows beyond the end of the buffer.
> > > > 
> > > > strlcpy() reads the entire source buffer first. This read
> > > > may exceed the destination size limit. This can be both inefficient
> > > > and lead to linear read overflows.
> > > > 
> > > > The safe replacement to both of these is to use strscpy() instead.
> > > > Add a new checkpatch warning which alerts the user on finding usage of
> > > > strcpy() or strlcpy().
> > > 
> > > I do not believe that strscpy is preferred over strcpy.
> > > 
> > > When the size of the output buffer is known to be larger
> > > than the input, strcpy is faster.
> > > 
> > > There are about 2k uses of strcpy.
> > > Is there a use where strcpy use actually matters?
> > > I don't know offhand...
> > > 
> > > But I believe compilers do not optimize away the uses of strscpy
> > > to a simple memcpy like they do for strcpy with a const from
> > > 
> > >         strcpy(foo, "bar");
> > > 
> > 
> > Yes the optimization here definitely helps. So in case the programmer
> > knows that the destination buffer is always larger, then strcpy() should be
> > preferred? I think the documentation might have been too strict about
> > strcpy() uses here:
> > 
> > Documentation/process/deprecated.rst:
> > "strcpy() performs no bounds checking on the destination buffer. This
> > could result in linear overflows beyond the end of the buffer, leading to
> > all kinds of misbehaviors. While `CONFIG_FORTIFY_SOURCE=y` and various
> > compiler flags help reduce the risk of using this function, there is
> > no good reason to add new uses of this function. The safe replacement
> > is strscpy(),..."
> 
> Kees/Jonathan:
> 
> Perhaps this text is overly restrictive.
> 
> There are ~2k uses of strcpy in the kernel.
> 
> About half of these are where the buffer length of foo is known and the
> use is 'strcpy(foo, "bar")' so the compiler converts/optimizes away the
> strcpy to memcpy and may not even put "bar" into the string table.
> 
> I believe strscpy uses do not have this optimization.
> 
> Is there a case where the runtime costs actually matters?
> I expect so.

The original goal was to use another helper that worked on static
strings like this. Linus rejected that idea, so we're in a weird place.
I think we could perhaps build a strcpy() replacement that requires
compile-time validated arguments, and to break the build if not.

i.e.

given:
	char array[8];
	char *ptr;

allow:


	strcpy(array, "1234567");

disallow:

	strcpy(array, "12345678");	/* too long */
	strcpy(array, src);		/* not optimized, so use strscpy? */
	strcpy(ptr, "1234567");		/* unknown destination size */
	strcpy(ptr, src);		/* unknown destination size */

What do you think?

-- 
Kees Cook
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] deprecated.rst: deprecated strcpy ? (was: [PATCH] checkpatch: add a new check for strcpy/strlcpy uses)

[Joe Perches]

On Thu, 2021-01-07 at 13:16 -0800, Kees Cook wrote:
> On Tue, Jan 05, 2021 at 01:28:18AM -0800, Joe Perches wrote:
> > On Tue, 2021-01-05 at 14:29 +0530, Dwaipayan Ray wrote:
> > > On Tue, Jan 5, 2021 at 2:14 PM Joe Perches <joe@perches.com> wrote:
> > > > 
> > > > On Tue, 2021-01-05 at 13:53 +0530, Dwaipayan Ray wrote:
> > > > > strcpy() performs no bounds checking on the destination buffer.
> > > > > This could result in linear overflows beyond the end of the buffer.
> > > > > 
> > > > > strlcpy() reads the entire source buffer first. This read
> > > > > may exceed the destination size limit. This can be both inefficient
> > > > > and lead to linear read overflows.
> > > > > 
> > > > > The safe replacement to both of these is to use strscpy() instead.
> > > > > Add a new checkpatch warning which alerts the user on finding usage of
> > > > > strcpy() or strlcpy().
> > > > 
> > > > I do not believe that strscpy is preferred over strcpy.
> > > > 
> > > > When the size of the output buffer is known to be larger
> > > > than the input, strcpy is faster.
> > > > 
> > > > There are about 2k uses of strcpy.
> > > > Is there a use where strcpy use actually matters?
> > > > I don't know offhand...
> > > > 
> > > > But I believe compilers do not optimize away the uses of strscpy
> > > > to a simple memcpy like they do for strcpy with a const from
> > > > 
> > > >         strcpy(foo, "bar");
> > > > 
> > > 
> > > Yes the optimization here definitely helps. So in case the programmer
> > > knows that the destination buffer is always larger, then strcpy() should be
> > > preferred? I think the documentation might have been too strict about
> > > strcpy() uses here:
> > > 
> > > Documentation/process/deprecated.rst:
> > > "strcpy() performs no bounds checking on the destination buffer. This
> > > could result in linear overflows beyond the end of the buffer, leading to
> > > all kinds of misbehaviors. While `CONFIG_FORTIFY_SOURCE=y` and various
> > > compiler flags help reduce the risk of using this function, there is
> > > no good reason to add new uses of this function. The safe replacement
> > > is strscpy(),..."
> > 
> > Kees/Jonathan:
> > 
> > Perhaps this text is overly restrictive.
> > 
> > There are ~2k uses of strcpy in the kernel.
> > 
> > About half of these are where the buffer length of foo is known and the
> > use is 'strcpy(foo, "bar")' so the compiler converts/optimizes away the
> > strcpy to memcpy and may not even put "bar" into the string table.
> > 
> > I believe strscpy uses do not have this optimization.
> > 
> > Is there a case where the runtime costs actually matters?
> > I expect so.
> 
> The original goal was to use another helper that worked on static
> strings like this. Linus rejected that idea, so we're in a weird place.
> I think we could perhaps build a strcpy() replacement that requires
> compile-time validated arguments, and to break the build if not.
> 
> i.e.
> 
> given:
> 	char array[8];
> 	char *ptr;
> 
> allow:
> 
> 
> 	strcpy(array, "1234567");
> 
> disallow:
> 
> 	strcpy(array, "12345678");	/* too long */
> 	strcpy(array, src);		/* not optimized, so use strscpy? */
> 	strcpy(ptr, "1234567");		/* unknown destination size */
> 	strcpy(ptr, src);		/* unknown destination size */

I think that's not a good idea as it's not a generic equivalent of the
string.h code.

I still like the stracpy variant I proposed:

https://lore.kernel.org/lkml/24bb53c57767c1c2a8f266c305a670f7@sk2.org/T/#m0627aa770a076af1937cb5c610ed71dab3f1da72
https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/

Linus liked a variant he called copy_string:

https://lore.kernel.org/lkml/CAHk-=wg8vLmmwTGhXM51NpSWJW8RFEAKoXxG0Hu_Q9Uwbjj8kw@mail.gmail.com/

I think the cocci scripts that convert:

	strlcpy -> strscpy (only when return value unused)
	str<sln>cpy(array, "string") -> stracpy(foo, "string")
	s[cn]printf -> sysfs_emit

would leave relatively few uses of strcpy and sprintf variants and would
make it much easier to analyze the remainder uses for potential overflows.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] deprecated.rst: deprecated strcpy ? (was: [PATCH] checkpatch: add a new check for strcpy/strlcpy uses)

[David Laight]

From: Joe Perches
> Sent: 08 January 2021 00:52
...
> > The original goal was to use another helper that worked on static
> > strings like this. Linus rejected that idea, so we're in a weird place.
> > I think we could perhaps build a strcpy() replacement that requires
> > compile-time validated arguments, and to break the build if not.
> >
> > i.e.
> >
> > given:
> > 	char array[8];
> > 	char *ptr;
> >
> > allow:
> >
> >
> > 	strcpy(array, "1234567");
> >
> > disallow:
> >
> > 	strcpy(array, "12345678");	/* too long */
> > 	strcpy(array, src);		/* not optimized, so use strscpy? */
> > 	strcpy(ptr, "1234567");		/* unknown destination size */
> > 	strcpy(ptr, src);		/* unknown destination size */
> 
> I think that's not a good idea as it's not a generic equivalent of the
> string.h code.
> 
> I still like the stracpy variant I proposed:
> 
> https://lore.kernel.org/lkml/24bb53c57767c1c2a8f266c305a670f7@sk2.org/T/#m0627aa770a076af1937cb5c610ed
> 71dab3f1da72
> https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/
> 
> Linus liked a variant he called copy_string:
> 
> https://lore.kernel.org/lkml/CAHk-=wg8vLmmwTGhXM51NpSWJW8RFEAKoXxG0Hu_Q9Uwbjj8kw@mail.gmail.com/
> 
> I think the cocci scripts that convert:
> 
> 	strlcpy -> strscpy (only when return value unused)
> 	str<sln>cpy(array, "string") -> stracpy(foo, "string")
> 	s[cn]printf -> sysfs_emit
> 
> would leave relatively few uses of strcpy and sprintf variants and would
> make it much easier to analyze the remainder uses for potential overflows.

The advantage of allowing strcpy() but only when the when it can be
converted into a non-overflowing memcpy() is that you know that the
copies never get truncated.

The next round of string copy errors could easily by the 'silent truncation'
ones - so using such a strcpy() will cut down the next audit.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] deprecated.rst: deprecated strcpy ? (was: [PATCH] checkpatch: add a new check for strcpy/strlcpy uses)

[Kees Cook]

On Thu, Jan 07, 2021 at 04:51:31PM -0800, Joe Perches wrote:
> I still like the stracpy variant I proposed:
> 
> https://lore.kernel.org/lkml/24bb53c57767c1c2a8f266c305a670f7@sk2.org/T/#m0627aa770a076af1937cb5c610ed71dab3f1da72
> https://lore.kernel.org/lkml/CAHk-=wgqQKoAnhmhGE-2PBFt7oQs9LLAATKbYa573UO=DPBE0Q@mail.gmail.com/
> 
> Linus liked a variant he called copy_string:
> 
> https://lore.kernel.org/lkml/CAHk-=wg8vLmmwTGhXM51NpSWJW8RFEAKoXxG0Hu_Q9Uwbjj8kw@mail.gmail.com/
> 
> I think the cocci scripts that convert:
> 
> 	strlcpy -> strscpy (only when return value unused)
> 	str<sln>cpy(array, "string") -> stracpy(foo, "string")
> 	s[cn]printf -> sysfs_emit
> 
> would leave relatively few uses of strcpy and sprintf variants and would
> make it much easier to analyze the remainder uses for potential overflows.

I think that would be lovely; yes. :)

-- 
Kees Cook
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH] checkpatch: add a new check for strcpy/strlcpy uses

[David Laight]

From: Joe Perches
> Sent: 05 January 2021 08:44
> 
> On Tue, 2021-01-05 at 13:53 +0530, Dwaipayan Ray wrote:
> > strcpy() performs no bounds checking on the destination buffer.
> > This could result in linear overflows beyond the end of the buffer.
> >
> > strlcpy() reads the entire source buffer first. This read
> > may exceed the destination size limit. This can be both inefficient
> > and lead to linear read overflows.
> >
> > The safe replacement to both of these is to use strscpy() instead.
> > Add a new checkpatch warning which alerts the user on finding usage of
> > strcpy() or strlcpy().
> 
> I do not believe that strscpy is preferred over strcpy.
> 
> When the size of the output buffer is known to be larger
> than the input, strcpy is faster.
> 
> There are about 2k uses of strcpy.
> Is there a use where strcpy use actually matters?
> I don't know offhand...
> 
> But I believe compilers do not optimize away the uses of strscpy
> to a simple memcpy like they do for strcpy with a const from
> 
> 	strcpy(foo, "bar");

It ought to be possible to convert:
	strscpy(foo, "bar", constant_sz)
to a memcpy() within the .h file.

Similarly it should be possible to error
	strcpy(foo, "bar")
Unless foo is large enough and "bar" is constant.

After all with a length check
	strcpy(foo, "bar")
is actually safer than
	strspy(foo, "bar", sizeof foo)
because there is less room for error.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees