[PATCH v4] Added warnings in checkpatch.pl script to :

[PATCH v4] Added warnings in checkpatch.pl script to :

[NitinGote]

From: Nitin Gote <nitin.r.gote@intel.com>

1. Deprecate strcpy() in favor of strscpy().
2. Deprecate strlcpy() in favor of strscpy().
3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().

Updated strncpy() section in Documentation/process/deprecated.rst
to cover strscpy_pad() case.

Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
---
 Change log:
 v1->v2
 - For string related apis, created different %deprecated_string_api
   and these will get emitted at CHECK Level using command line option
   -f/--file to avoid bad patched from novice script users.

 v2->v3
 - Avoided use of $check in implementation.
 - Incorporated trivial comments.

 v3->v4
 - Incorporated comment by removing "c:func:"

 Documentation/process/deprecated.rst |  6 +++---
 scripts/checkpatch.pl                | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 49e0f64a3427..0fb37ebe3ad9 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -93,9 +93,9 @@ will be NUL terminated. This can lead to various linear read overflows
 and other misbehavior due to the missing termination. It also NUL-pads the
 destination buffer if the source contents are shorter than the destination
 buffer size, which may be a needless performance penalty for callers using
-only NUL-terminated strings. The safe replacement is :c:func:`strscpy`.
-(Users of :c:func:`strscpy` still needing NUL-padding will need an
-explicit :c:func:`memset` added.)
+only NUL-terminated strings. In this case, the safe replacement is
+`strscpy()`. If, however, the destination buffer still needs NUL-padding,
+the safe replacement is `strscpy_pad()`.

 If a caller is using non-NUL-terminated strings, :c:func:`strncpy()` can
 still be used, but destinations should be marked with the `__nonstring
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bb28b178d929..e6fbf4cf4be4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) {
 }
 $deprecated_apis_search = "(?:${deprecated_apis_search})";

+our %deprecated_string_apis = (
+        "strcpy"				=> "strscpy",
+        "strlcpy"				=> "strscpy",
+        "strncpy"				=> "strscpy, strscpy_pad or for non-NUL-terminated strings, strncpy() can still be used, but destinations should be marked with the __nonstring",
+);
+
+#Create a search pattern for all these strings apis to speed up a loop below
+our $deprecated_string_apis_search = "";
+foreach my $entry (keys %deprecated_string_apis) {
+        $deprecated_string_apis_search .= '|' if ($deprecated_string_apis_search ne "");
+        $deprecated_string_apis_search .= $entry;
+}
+$deprecated_string_apis_search = "(?:${deprecated_string_apis_search})";
+
 our $mode_perms_world_writable = qr{
 	S_IWUGO		|
 	S_IWOTH		|
@@ -6446,6 +6460,16 @@ sub process {
 			     "Deprecated use of '$deprecated_api', prefer '$new_api' instead\n" . $herecurr);
 		}

+# check for string deprecated apis
+		if ($line =~ /\b($deprecated_string_apis_search)\b\s*\(/) {
+			my $deprecated_string_api = $1;
+			my $new_api = $deprecated_string_apis{$deprecated_string_api};
+			my $msg_level = \&WARN;
+			$msg_level = \&CHK if ($file);
+			&{$msg_level}("DEPRECATED_API",
+				      "Deprecated use of '$deprecated_string_api', prefer '$new_api' instead\n" . $herecurr);
+		}
+
 # check for various structs that are normally const (ops, kgdb, device_tree)
 # and avoid what seem like struct definitions 'struct foo {'
 		if ($line !~ /\bconst\b/ &&
--
2.17.1

Re: [PATCH v4] Added warnings in checkpatch.pl script to :

[Jonathan Corbet]

On Tue,  9 Jul 2019 21:18:06 +0530
NitinGote <nitin.r.gote@intel.com> wrote:

> From: Nitin Gote <nitin.r.gote@intel.com>
> 
> 1. Deprecate strcpy() in favor of strscpy().
> 2. Deprecate strlcpy() in favor of strscpy().
> 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> 
> Updated strncpy() section in Documentation/process/deprecated.rst
> to cover strscpy_pad() case.
> 
> Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
> ---
>  Change log:
>  v1->v2
>  - For string related apis, created different %deprecated_string_api
>    and these will get emitted at CHECK Level using command line option
>    -f/--file to avoid bad patched from novice script users.
> 
>  v2->v3
>  - Avoided use of $check in implementation.
>  - Incorporated trivial comments.
> 
>  v3->v4
>  - Incorporated comment by removing "c:func:"

But you ignored the comment asking for a proper subject line on the
patch.  

Also,

> -only NUL-terminated strings. The safe replacement is :c:func:`strscpy`.
> -(Users of :c:func:`strscpy` still needing NUL-padding will need an
> -explicit :c:func:`memset` added.)
> +only NUL-terminated strings. In this case, the safe replacement is
> +`strscpy()`. If, however, the destination buffer still needs NUL-padding,
> +the safe replacement is `strscpy_pad()`.

Please make those just strscpy(), not `strscpy()`.  As I said, the right
thing will happen.

Thanks,

jon

Re: [PATCH v4] Added warnings in checkpatch.pl script to :

[Joe Perches]

On Tue, 2019-07-09 at 21:18 +0530, NitinGote wrote:
> From: Nitin Gote <nitin.r.gote@intel.com>
> 
> 1. Deprecate strcpy() in favor of strscpy().
> 2. Deprecate strlcpy() in favor of strscpy().
> 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> 
> Updated strncpy() section in Documentation/process/deprecated.rst
> to cover strscpy_pad() case.

Please slow down your patch submission rate for
this instance and respond appropriately to the
comments you've been given.

This stuff is not critical bug fixing.

The subject could be something like:

Subject: [PATCH v#] Documentation/checkpatch: Prefer strscpy over strcpy/strlcpy

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) {
>  }
>  $deprecated_apis_search = "(?:${deprecated_apis_search})";
> 
> +our %deprecated_string_apis = (
> +        "strcpy"				=> "strscpy",
> +        "strlcpy"				=> "strscpy",
> +        "strncpy"				=> "strscpy, strscpy_pad or for non-NUL-terminated strings, strncpy() can still be used, but destinations should be marked with the __nonstring",

'the' is not necessary.

There could likely also be a strscat created for
strcat, strlcat and strncat.

btw:

There were several defects in the kernel for misuses
of strlcpy.

Did you or anyone else have an opinion on stracpy
to avoid duplicating the first argument in a
sizeof()?

	strlcpy(foo, bar, sizeof(foo))
to
	stracpy(foo, bar)

where foo must be char array compatible ?

https://lore.kernel.org/lkml/d1524130f91d7cfd61bc736623409693d2895f57.camel@perches.com/

RE: [PATCH v4] Added warnings in checkpatch.pl script to :

[Gote, Nitin R]

> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Tuesday, July 9, 2019 9:40 PM
> To: Gote, Nitin R <nitin.r.gote@intel.com>; corbet@lwn.net
> Cc: akpm@linux-foundation.org; apw@canonical.com;
> keescook@chromium.org; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH v4] Added warnings in checkpatch.pl script to :
> 
> On Tue, 2019-07-09 at 21:18 +0530, NitinGote wrote:
> > From: Nitin Gote <nitin.r.gote@intel.com>
> >
> > 1. Deprecate strcpy() in favor of strscpy().
> > 2. Deprecate strlcpy() in favor of strscpy().
> > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> >
> > Updated strncpy() section in Documentation/process/deprecated.rst
> > to cover strscpy_pad() case.
> 
> Please slow down your patch submission rate for this instance and respond
> appropriately to the comments you've been given.

Sure, I will explore this things more. And sorry, I missed to incorporate one comment. 
I will take care of such things.

> 
> This stuff is not critical bug fixing.
> 
Noted.

> The subject could be something like:
> 
> Subject: [PATCH v#] Documentation/checkpatch: Prefer strscpy over
> strcpy/strlcpy
> 

How about this  :
Subject: [PATCH v#] Doc/checkpatch: Prefer strscpy/strscpy_pad over strcpy/strlcpy/strncpy

> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -605,6 +605,20 @@ foreach my $entry (keys %deprecated_apis) {  }
> > $deprecated_apis_search = "(?:${deprecated_apis_search})";
> >
> > +our %deprecated_string_apis = (
> > +        "strcpy"				=> "strscpy",
> > +        "strlcpy"				=> "strscpy",
> > +        "strncpy"				=> "strscpy, strscpy_pad or
> for non-NUL-terminated strings, strncpy() can still be used, but destinations
> should be marked with the __nonstring",
> 
> 'the' is not necessary.

Noted.

> 
> There could likely also be a strscat created for strcat, strlcat and strncat.
>

I have not found reference for strscat in kernel.
Could you please give any reference for strscat ?
 
> btw:
> 
> There were several defects in the kernel for misuses of strlcpy.
> 
> Did you or anyone else have an opinion on stracpy to avoid duplicating the
> first argument in a sizeof()?
> 
> 	strlcpy(foo, bar, sizeof(foo))
> to
> 	stracpy(foo, bar)
> 
> where foo must be char array compatible ?
> 
> https://lore.kernel.org/lkml/d1524130f91d7cfd61bc736623409693d2895f57.
> camel@perches.com/
> 
>

As I understood, your trying to give new interface like stracpy(), to avoid duplication of first 
argument in a sizeof(), we can also make it more robust for users by adding check or warn in 
checkpatch.pl to prefer stracpy().

Did you or anyone has opinion on this ?


Thanks,
Nitin Gote