[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

[Linux-kernel-mentees] [PATCH] checkpatch: trivial style fixes

[Dwaipayan Ray]

Indentations should use tabs wherever possible.
Replace spaces by tabs for indents.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b212f5f12ca6..d6a4d25b0972 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2973,7 +2973,7 @@ sub process {
 				}
 				if (!defined $lines[$linenr]) {
 					WARN("BAD_SIGN_OFF",
-                                             "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
+					     "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline);
 				} elsif ($rawlines[$linenr] !~ /^\s*signed-off-by:\s*(.*)/i) {
 					WARN("BAD_SIGN_OFF",
 					     "Co-developed-by: must be immediately followed by Signed-off-by:\n" . "$here\n" . $rawline . "\n" .$rawlines[$linenr]);
@@ -2996,8 +2996,8 @@ sub process {
 			if (ERROR("GERRIT_CHANGE_ID",
 			          "Remove Gerrit Change-Id's before submitting upstream\n" . $herecurr) &&
 			    $fix) {
-                                fix_delete_line($fixlinenr, $rawline);
-                        }
+				fix_delete_line($fixlinenr, $rawline);
+			}
 		}
 
 # Check if the commit log is in a possible stack dump
@@ -3239,10 +3239,10 @@ sub process {
 				next if ($start_char =~ /^\S$/);
 				next if (index(" \t.,;?!", $end_char) == -1);
 
-                                # avoid repeating hex occurrences like 'ff ff fe 09 ...'
-                                if ($first =~ /\b[0-9a-f]{2,}\b/i) {
-                                        next if (!exists($allow_repeated_words{lc($first)}));
-                                }
+				# avoid repeating hex occurrences like 'ff ff fe 09 ...'
+				if ($first =~ /\b[0-9a-f]{2,}\b/i) {
+					next if (!exists($allow_repeated_words{lc($first)}));
+				}
 
 				if (WARN("REPEATED_WORD",
 					 "Possible repeated word: '$first'\n" . $herecurr) &&
@@ -4426,7 +4426,7 @@ sub process {
 			WARN("STATIC_CONST_CHAR_ARRAY",
 			     "char * array declaration might be better as static const\n" .
 				$herecurr);
-               }
+		}
 
 # check for sizeof(foo)/sizeof(foo[0]) that could be ARRAY_SIZE(foo)
 		if ($line =~ m@\bsizeof\s*\(\s*($Lval)\s*\)@) {
@@ -5279,7 +5279,7 @@ sub process {
 		    $lines[$linenr - 3] !~ /^[ +]\s*$Ident\s*:/) {
 			WARN("RETURN_VOID",
 			     "void function return statements are not generally useful\n" . $hereprev);
-               }
+		}
 
 # if statements using unnecessary parentheses - ie: if ((foo == bar))
 		if ($perl_version_ok &&
-- 
2.27.0

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

[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

[Lukas Bulwahn]

On Mon, Jan 4, 2021 at 2:25 PM Dwaipayan Ray <dwaipayanray1@gmail.com> 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().
>
> Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> ---

I remember Joe has already created a patch for that over Christmas
break; check lkml before sending this.

Other than that, looks good.

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

[Dwaipayan Ray]

On Tue, Jan 5, 2021 at 1:32 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> On Mon, Jan 4, 2021 at 2:25 PM Dwaipayan Ray <dwaipayanray1@gmail.com> 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().
> >
> > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > ---
>
> I remember Joe has already created a patch for that over Christmas
> break; check lkml before sending this.
>
> Other than that, looks good.
>
> Lukas

Yes I found it:
https://lore.kernel.org/lkml/22b393d1790bb268769d0bab7bacf0866dcb0c14.camel@perches.com/

He has converted the uses in code. But I don't think he has created
the checkpatch rule yet. I will try sending it out to him.

Thank you,
Dwaipayan.
_______________________________________________
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

[Lukas Bulwahn]

On Tue, Jan 5, 2021 at 9:19 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Tue, Jan 5, 2021 at 1:32 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >
> > On Mon, Jan 4, 2021 at 2:25 PM Dwaipayan Ray <dwaipayanray1@gmail.com> 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().
> > >
> > > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > > ---
> >
> > I remember Joe has already created a patch for that over Christmas
> > break; check lkml before sending this.
> >
> > Other than that, looks good.
> >
> > Lukas
>
> Yes I found it:
> https://lore.kernel.org/lkml/22b393d1790bb268769d0bab7bacf0866dcb0c14.camel@perches.com/
>
> He has converted the uses in code. But I don't think he has created
> the checkpatch rule yet. I will try sending it out to him.
>

Joe pointed you already to the commit. So I think your change is obsolete.

And I fully agree with Joe. strcpy is perfectly fine, when it is clear
from the use that boundaries do not need to be checked.


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