checkpatch: add a new check for strcpy/strlcpy uses

PATCH In review Dwaipayan Ray 4 participants 9 messages Jan 5, 2021 – Jan 8, 2021 Checkpatch

Share & tools

Dwaipayan Ray

author

2021-01-05 08:23 UTC #0 lore raw

PATCH 1/1 checkpatch: add a new check for strcpy/strlcpy uses

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 <[email protected]>

---
 scripts/checkpatch.pl | 7 +++++++
 1 file changed, 7 insertions(+)

scripts/checkpatch.pl +7 -0

index d6a4d25b0972..0003fd9de62c 100755
@@ -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 &&

Signature · 6 lines

2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Joe Perches

maintainer

2021-01-05 08:44 UTC #1 lore raw

Quoted 10 lines · 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().

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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Dwaipayan Ray

author

2021-01-05 09:00 UTC #2 lore raw

Quoted 18 lines · Joe Perches

Quoted 10 lines · 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().
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.

Quoted 9 lines

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(),..."

Quoted 4 lines

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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Joe Perches

maintainer

2021-01-05 09:28 UTC #3 lore raw

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

Quoted 34 lines · Dwaipayan Ray

Quoted 18 lines · Joe Perches
Quoted 10 lines · 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().
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.
Quoted 9 lines
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:

Quoted 7 lines

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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Kees Cook

2021-01-07 21:16 UTC #4 lore raw

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

Quoted 49 lines · Joe Perches

Quoted 34 lines · Dwaipayan Ray
Quoted 18 lines · Joe Perches
Quoted 10 lines · 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().
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.
Quoted 9 lines
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:
Quoted 7 lines
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.

Quoted 8 lines

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?

Signature · 5 lines

Kees Cook
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Joe Perches

maintainer

2021-01-08 00:51 UTC #5 lore raw

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

Quoted 73 lines · Kees Cook

Quoted 49 lines · Joe Perches
Quoted 34 lines · Dwaipayan Ray
Quoted 18 lines · Joe Perches
Quoted 10 lines · 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().
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.
Quoted 9 lines
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:
Quoted 7 lines
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.
Quoted 8 lines
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:

Quoted 8 lines

	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/[email protected]/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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

David Laight

2021-01-08 10:06 UTC #6 lore raw

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

From: Joe Perches

Quoted 1 line

Sent: 08 January 2021 00:52

...

Quoted 36 lines

Quoted 14 lines
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:
Quoted 8 lines
	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/[email protected]/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/

Quoted 8 lines

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
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Kees Cook

2021-01-08 19:48 UTC #7 lore raw

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

In reply to Joe Perches

Quoted 9 lines · Joe Perches

I still like the stracpy variant I proposed:
https://lore.kernel.org/lkml/[email protected]/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/

Quoted 8 lines

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. :)

Signature · 5 lines

Kees Cook
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

David Laight

2021-01-05 10:22 UTC #8 lore raw

In reply to Joe Perches

From: Joe Perches

Quoted 19 lines

Sent: 05 January 2021 08:44
Quoted 10 lines · 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().
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.

Quoted 8 lines

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)