[PATCH] checkpatch: Added warnings in favor of strscpy().

[PATCH] checkpatch: Added warnings in favor of strscpy().

[Nitin Gote]

Added warnings in checkpatch.pl script to :

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>
---
 This patch is already reviewed by mailing list
 kernel-hardening@lists.openwall.com. Refer below link
 <https://www.openwall.com/lists/kernel-hardening/2019/07/03/4>
Acked-by: Kees Cook <keescook@chromium.org>

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

diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 49e0f64..f564de3 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
+:c:func:`strscpy`. If, however, the destination buffer still needs
+NUL-padding, the safe replacement is :c:func:`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 342c7c7..3d80967 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -595,6 +595,11 @@ our %deprecated_apis = (
 	"rcu_barrier_sched"			=> "rcu_barrier",
 	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
 	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
+	"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 to speed up a loop below
-- 
2.7.4

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> Added warnings in checkpatch.pl script to :
> 
> 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>

OK, for whatever reason, this when into a spam folder.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -595,6 +595,11 @@ our %deprecated_apis = (
>  	"rcu_barrier_sched"			=> "rcu_barrier",
>  	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
>  	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
> +	"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",
>  );

$ git grep -w strcpy | wc -l
2239
$ git grep -w strlcpy | wc -l
1760
$ git grep -w strncpy | wc -l
839

These functions are _really_ commonly used in the kernel.

This should probably be a different %deprecated_string_api
and these should probably not be emitted at WARN level
when using command line option -f/--file but at CHECK level
so that novice script users just don't send bad patches.

Also, perhaps there could be some macro for the relatively
commonly used

	strscpy(foo, bar, sizeof(foo))
and
	strlcpy(foo, bar, sizeof(foo))

so argument 1 doesn't have to be repeated in the sizeof()

Something like:

#define stracpy(to, from)					\
({								\
	size_t size = ARRAY_SIZE(to);				\
	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
								\
	strscpy(to, from, size);				\
})

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> Added warnings in checkpatch.pl script to :
> 
> 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>

OK, for whatever reason, this when into a spam folder.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -595,6 +595,11 @@ our %deprecated_apis = (
>  	"rcu_barrier_sched"			=> "rcu_barrier",
>  	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
>  	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
> +	"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",
>  );

$ git grep -w strcpy | wc -l
2239
$ git grep -w strlcpy | wc -l
1760
$ git grep -w strncpy | wc -l
839

These functions are _really_ commonly used in the kernel.

This should probably be a different %deprecated_string_api
and these should probably not be emitted at WARN level
when using command line option -f/--file but at CHECK level
so that novice script users just don't send bad patches.

Also, perhaps there could be some macro for the relatively
commonly used

	strscpy(foo, bar, sizeof(foo))
and
	strlcpy(foo, bar, sizeof(foo))

so argument 1 doesn't have to be repeated in the sizeof()

Something like:

#define stracpy(to, from)					\
({								\
	size_t size = ARRAY_SIZE(to);				\
	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
								\
	strscpy(to, from, size);				\
})

[RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > Added warnings in checkpatch.pl script to :
> > 
> > 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.

[]

I sent a patch series for some strscpy/strlcpy misuses.

How about adding a macro helper to avoid the misuses like:
---
 include/linux/string.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 4deb11f7976b..ef01bd6f19df 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
 /* Wraps calls to strscpy()/memset(), no arch specific code required */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count);
 
+#define stracpy(to, from)					\
+({								\
+	size_t size = ARRAY_SIZE(to);				\
+	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
+								\
+	strscpy(to, from, size);				\
+})
+
+#define stracpy_pad(to, from)					\
+({								\
+	size_t size = ARRAY_SIZE(to);				\
+	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
+								\
+	strscpy_pad(to, from, size);				\
+})
+
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif

[RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > Added warnings in checkpatch.pl script to :
> > 
> > 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.

[]

I sent a patch series for some strscpy/strlcpy misuses.

How about adding a macro helper to avoid the misuses like:
---
 include/linux/string.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 4deb11f7976b..ef01bd6f19df 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
 /* Wraps calls to strscpy()/memset(), no arch specific code required */
 ssize_t strscpy_pad(char *dest, const char *src, size_t count);
 
+#define stracpy(to, from)					\
+({								\
+	size_t size = ARRAY_SIZE(to);				\
+	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
+								\
+	strscpy(to, from, size);				\
+})
+
+#define stracpy_pad(to, from)					\
+({								\
+	size_t size = ARRAY_SIZE(to);				\
+	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
+								\
+	strscpy_pad(to, from, size);				\
+})
+
 #ifndef __HAVE_ARCH_STRCAT
 extern char * strcat(char *, const char *);
 #endif

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Kees Cook]

On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > Added warnings in checkpatch.pl script to :
> > > 
> > > 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.
> 
> []
> 
> I sent a patch series for some strscpy/strlcpy misuses.
> 
> How about adding a macro helper to avoid the misuses like:
> ---
>  include/linux/string.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 4deb11f7976b..ef01bd6f19df 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
>  /* Wraps calls to strscpy()/memset(), no arch specific code required */
>  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
>  
> +#define stracpy(to, from)					\
> +({								\
> +	size_t size = ARRAY_SIZE(to);				\
> +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> +								\
> +	strscpy(to, from, size);				\
> +})
> +
> +#define stracpy_pad(to, from)					\
> +({								\
> +	size_t size = ARRAY_SIZE(to);				\
> +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> +								\
> +	strscpy_pad(to, from, size);				\
> +})
> +
>  #ifndef __HAVE_ARCH_STRCAT
>  extern char * strcat(char *, const char *);
>  #endif

This seems like a reasonable addition, yes. I think Coccinelle might
actually be able to find all the existing strscpy(dst, src, sizeof(dst))
cases to jump-start this conversion.

Devil's advocate: this adds yet more string handling functions... will
this cause even more confusion?

-- 
Kees Cook

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > Added warnings in checkpatch.pl script to :
> > > > 
> > > > 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.
> > 
> > []
> > 
> > I sent a patch series for some strscpy/strlcpy misuses.
> > 
> > How about adding a macro helper to avoid the misuses like:
> > ---
> >  include/linux/string.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 4deb11f7976b..ef01bd6f19df 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> >  /* Wraps calls to strscpy()/memset(), no arch specific code required */
> >  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> >  
> > +#define stracpy(to, from)					\
> > +({								\
> > +	size_t size = ARRAY_SIZE(to);				\
> > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > +								\
> > +	strscpy(to, from, size);				\
> > +})
> > +
> > +#define stracpy_pad(to, from)					\
> > +({								\
> > +	size_t size = ARRAY_SIZE(to);				\
> > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > +								\
> > +	strscpy_pad(to, from, size);				\
> > +})
> > +
> >  #ifndef __HAVE_ARCH_STRCAT
> >  extern char * strcat(char *, const char *);
> >  #endif
> 
> This seems like a reasonable addition, yes. I think Coccinelle might
> actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> cases to jump-start this conversion.

I did that.  It works.  It's a lot of conversions.

$ cat str.cpy.cocci
@@
expression e1;
expression e2;
@@

- strscpy(e1, e2, sizeof(e1))
+ stracpy(e1, e2)

@@
expression e1;
expression e2;
@@

- strlcpy(e1, e2, sizeof(e1))
+ stracpy(e1, e2)

> Devil's advocate: this adds yet more string handling functions... will
> this cause even more confusion?

Documentation is good.
Actual in-kernel use and examples better.

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > Added warnings in checkpatch.pl script to :
> > > > 
> > > > 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.
> > 
> > []
> > 
> > I sent a patch series for some strscpy/strlcpy misuses.
> > 
> > How about adding a macro helper to avoid the misuses like:
> > ---
> >  include/linux/string.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 4deb11f7976b..ef01bd6f19df 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> >  /* Wraps calls to strscpy()/memset(), no arch specific code required */
> >  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> >  
> > +#define stracpy(to, from)					\
> > +({								\
> > +	size_t size = ARRAY_SIZE(to);				\
> > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > +								\
> > +	strscpy(to, from, size);				\
> > +})
> > +
> > +#define stracpy_pad(to, from)					\
> > +({								\
> > +	size_t size = ARRAY_SIZE(to);				\
> > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > +								\
> > +	strscpy_pad(to, from, size);				\
> > +})
> > +
> >  #ifndef __HAVE_ARCH_STRCAT
> >  extern char * strcat(char *, const char *);
> >  #endif
> 
> This seems like a reasonable addition, yes. I think Coccinelle might
> actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> cases to jump-start this conversion.

I did that.  It works.  It's a lot of conversions.

$ cat str.cpy.cocci
@@
expression e1;
expression e2;
@@

- strscpy(e1, e2, sizeof(e1))
+ stracpy(e1, e2)

@@
expression e1;
expression e2;
@@

- strlcpy(e1, e2, sizeof(e1))
+ stracpy(e1, e2)

> Devil's advocate: this adds yet more string handling functions... will
> this cause even more confusion?

Documentation is good.
Actual in-kernel use and examples better.

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > > Added warnings in checkpatch.pl script to :
> > > > > 
> > > > > 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.
> > > 
> > > []
> > > 
> > > I sent a patch series for some strscpy/strlcpy misuses.
> > > 
> > > How about adding a macro helper to avoid the misuses like:
> > > ---
> > >  include/linux/string.h | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > index 4deb11f7976b..ef01bd6f19df 100644
> > > --- a/include/linux/string.h
> > > +++ b/include/linux/string.h
> > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > >  /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > >  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> > >  
> > > +#define stracpy(to, from)					\
> > > +({								\
> > > +	size_t size = ARRAY_SIZE(to);				\
> > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > +								\
> > > +	strscpy(to, from, size);				\
> > > +})
> > > +
> > > +#define stracpy_pad(to, from)					\
> > > +({								\
> > > +	size_t size = ARRAY_SIZE(to);				\
> > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > +								\
> > > +	strscpy_pad(to, from, size);				\
> > > +})
> > > +
> > >  #ifndef __HAVE_ARCH_STRCAT
> > >  extern char * strcat(char *, const char *);
> > >  #endif
> > 
> > This seems like a reasonable addition, yes. I think Coccinelle might
> > actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> > cases to jump-start this conversion.
> 
> I did that.  It works.  It's a lot of conversions.
> 
> $ cat str.cpy.cocci
> @@
> expression e1;
> expression e2;
> @@
> 
> - strscpy(e1, e2, sizeof(e1))
> + stracpy(e1, e2)
> 
> @@
> expression e1;
> expression e2;
> @@
> 
> - strlcpy(e1, e2, sizeof(e1))
> + stracpy(e1, e2)
> 
> > Devil's advocate: this adds yet more string handling functions... will
> > this cause even more confusion?
> 
> Documentation is good.
> Actual in-kernel use and examples better.

btw: I just ran this again and it produces:

$ spatch --in-place -sp-file str.cpy.cocci .
$ git checkout tools/
$ git diff --shortstat
 958 files changed, 2179 insertions(+), 2655 deletions(-)

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > > Added warnings in checkpatch.pl script to :
> > > > > 
> > > > > 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.
> > > 
> > > []
> > > 
> > > I sent a patch series for some strscpy/strlcpy misuses.
> > > 
> > > How about adding a macro helper to avoid the misuses like:
> > > ---
> > >  include/linux/string.h | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > index 4deb11f7976b..ef01bd6f19df 100644
> > > --- a/include/linux/string.h
> > > +++ b/include/linux/string.h
> > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > >  /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > >  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> > >  
> > > +#define stracpy(to, from)					\
> > > +({								\
> > > +	size_t size = ARRAY_SIZE(to);				\
> > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > +								\
> > > +	strscpy(to, from, size);				\
> > > +})
> > > +
> > > +#define stracpy_pad(to, from)					\
> > > +({								\
> > > +	size_t size = ARRAY_SIZE(to);				\
> > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > +								\
> > > +	strscpy_pad(to, from, size);				\
> > > +})
> > > +
> > >  #ifndef __HAVE_ARCH_STRCAT
> > >  extern char * strcat(char *, const char *);
> > >  #endif
> > 
> > This seems like a reasonable addition, yes. I think Coccinelle might
> > actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> > cases to jump-start this conversion.
> 
> I did that.  It works.  It's a lot of conversions.
> 
> $ cat str.cpy.cocci
> @@
> expression e1;
> expression e2;
> @@
> 
> - strscpy(e1, e2, sizeof(e1))
> + stracpy(e1, e2)
> 
> @@
> expression e1;
> expression e2;
> @@
> 
> - strlcpy(e1, e2, sizeof(e1))
> + stracpy(e1, e2)
> 
> > Devil's advocate: this adds yet more string handling functions... will
> > this cause even more confusion?
> 
> Documentation is good.
> Actual in-kernel use and examples better.

btw: I just ran this again and it produces:

$ spatch --in-place -sp-file str.cpy.cocci .
$ git checkout tools/
$ git diff --shortstat
 958 files changed, 2179 insertions(+), 2655 deletions(-)

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Kees Cook]

On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > > > Added warnings in checkpatch.pl script to :
> > > > > > 
> > > > > > 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.
> > > > 
> > > > []
> > > > 
> > > > I sent a patch series for some strscpy/strlcpy misuses.
> > > > 
> > > > How about adding a macro helper to avoid the misuses like:
> > > > ---
> > > >  include/linux/string.h | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > > 
> > > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > > index 4deb11f7976b..ef01bd6f19df 100644
> > > > --- a/include/linux/string.h
> > > > +++ b/include/linux/string.h
> > > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > > >  /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > > >  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> > > >  
> > > > +#define stracpy(to, from)					\
> > > > +({								\
> > > > +	size_t size = ARRAY_SIZE(to);				\
> > > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > > +								\
> > > > +	strscpy(to, from, size);				\
> > > > +})
> > > > +
> > > > +#define stracpy_pad(to, from)					\
> > > > +({								\
> > > > +	size_t size = ARRAY_SIZE(to);				\
> > > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > > +								\
> > > > +	strscpy_pad(to, from, size);				\
> > > > +})
> > > > +
> > > >  #ifndef __HAVE_ARCH_STRCAT
> > > >  extern char * strcat(char *, const char *);
> > > >  #endif
> > > 
> > > This seems like a reasonable addition, yes. I think Coccinelle might
> > > actually be able to find all the existing strscpy(dst, src, sizeof(dst))
> > > cases to jump-start this conversion.
> > 
> > I did that.  It works.  It's a lot of conversions.
> > 
> > $ cat str.cpy.cocci
> > @@
> > expression e1;
> > expression e2;
> > @@
> > 
> > - strscpy(e1, e2, sizeof(e1))
> > + stracpy(e1, e2)
> > 
> > @@
> > expression e1;
> > expression e2;
> > @@
> > 
> > - strlcpy(e1, e2, sizeof(e1))
> > + stracpy(e1, e2)
> > 
> > > Devil's advocate: this adds yet more string handling functions... will
> > > this cause even more confusion?
> > 
> > Documentation is good.
> > Actual in-kernel use and examples better.
> 
> btw: I just ran this again and it produces:
> 
> $ spatch --in-place -sp-file str.cpy.cocci .
> $ git checkout tools/
> $ git diff --shortstat
>  958 files changed, 2179 insertions(+), 2655 deletions(-)

Cool. Well, assuming no one hates this, let's do it. :) Can you send a
more complete patch with docs, etc? Maybe Linus will take it for late
in the next merge window, perhaps?

-- 
Kees Cook

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Matthew Wilcox]

On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote:
> On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
> > > > > On Thu, 2019-07-04 at 11:24 +0530, Nitin Gote wrote:
> > > > > > Added warnings in checkpatch.pl script to :
> > > > > > 
> > > > > > 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.
> > > > 
> > > > []
> > > > 
> > > > I sent a patch series for some strscpy/strlcpy misuses.
> > > > 
> > > > How about adding a macro helper to avoid the misuses like:
> > > > ---
> > > >  include/linux/string.h | 16 ++++++++++++++++
> > > >  1 file changed, 16 insertions(+)
> > > > 
> > > > diff --git a/include/linux/string.h b/include/linux/string.h
> > > > index 4deb11f7976b..ef01bd6f19df 100644
> > > > --- a/include/linux/string.h
> > > > +++ b/include/linux/string.h
> > > > @@ -35,6 +35,22 @@ ssize_t strscpy(char *, const char *, size_t);
> > > >  /* Wraps calls to strscpy()/memset(), no arch specific code required */
> > > >  ssize_t strscpy_pad(char *dest, const char *src, size_t count);
> > > >  
> > > > +#define stracpy(to, from)					\
> > > > +({								\
> > > > +	size_t size = ARRAY_SIZE(to);				\
> > > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > > +								\
> > > > +	strscpy(to, from, size);				\
> > > > +})

Where does the 'a' in 'stracpy' come from?  Googling around finds other
people using a function called stracpy, but it takes different arguments.
http://stracpy.blogspot.com/ takes a size argument, as does
https://docs.polserver.com/doxygen/html/d5/dce/stracpy_8cpp_source.html

The one in the 'Links' webbrowser (can't find a link to its source) seems
like a strdup clone.

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Mon, 2019-07-22 at 11:27 -0700, Matthew Wilcox wrote:
> On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote:
> > On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> > > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
[]
> > > > > +#define stracpy(to, from)					\
> > > > > +({								\
> > > > > +	size_t size = ARRAY_SIZE(to);				\
> > > > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > > > +								\
> > > > > +	strscpy(to, from, size);				\
> > > > > +})
> 
> Where does the 'a' in 'stracpy' come from?

No place in particular.

I used it because dst has to be an 'a'rray rather
than a pointer.

Re: [RFC PATCH] string.h: Add stracpy/stracpy_pad (was: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().)

[Joe Perches]

On Mon, 2019-07-22 at 11:27 -0700, Matthew Wilcox wrote:
> On Mon, Jul 22, 2019 at 10:58:15AM -0700, Joe Perches wrote:
> > On Mon, 2019-07-22 at 10:43 -0700, Joe Perches wrote:
> > > On Mon, 2019-07-22 at 10:33 -0700, Kees Cook wrote:
> > > > On Thu, Jul 04, 2019 at 05:15:57PM -0700, Joe Perches wrote:
> > > > > On Thu, 2019-07-04 at 13:46 -0700, Joe Perches wrote:
[]
> > > > > +#define stracpy(to, from)					\
> > > > > +({								\
> > > > > +	size_t size = ARRAY_SIZE(to);				\
> > > > > +	BUILD_BUG_ON(!__same_type(typeof(*to), char));		\
> > > > > +								\
> > > > > +	strscpy(to, from, size);				\
> > > > > +})
> 
> Where does the 'a' in 'stracpy' come from?

No place in particular.

I used it because dst has to be an 'a'rray rather
than a pointer.

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 16:28 -0600, Jonathan Corbet wrote:
> On Mon, 22 Jul 2019 15:24:33 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > If the functions themselves are fully defined in the .h file, I'd just add
> > > the kerneldoc there as well.  That's how it's usually done, and you want
> > > to keep the documentation and the prototypes together.  
> > 
> > In this case, it's a macro and yes, the kernel-doc could
> > easily be set around the macro in the .h, but my desire
> > is to keep all the string function kernel-doc output
> > together so it should be added to lib/string.c
> > 
> > Are you suggesting I move all the lib/string.c kernel-doc
> > to include/linux/string.h ?
> 
> If you want the *output* together, just put the kernel-doc directives
> together in the RST file that pulls it all in.  Or am I missing something
> here?

The negative of the kernel-doc separation of prototypes by .h
and .c files is that the ordering of the functions in the .rst
outout files doesn't make much logical sense.

stracpy is pretty far away from strscpy in the list of functions.

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 16:28 -0600, Jonathan Corbet wrote:
> On Mon, 22 Jul 2019 15:24:33 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > If the functions themselves are fully defined in the .h file, I'd just add
> > > the kerneldoc there as well.  That's how it's usually done, and you want
> > > to keep the documentation and the prototypes together.  
> > 
> > In this case, it's a macro and yes, the kernel-doc could
> > easily be set around the macro in the .h, but my desire
> > is to keep all the string function kernel-doc output
> > together so it should be added to lib/string.c
> > 
> > Are you suggesting I move all the lib/string.c kernel-doc
> > to include/linux/string.h ?
> 
> If you want the *output* together, just put the kernel-doc directives
> together in the RST file that pulls it all in.  Or am I missing something
> here?

The negative of the kernel-doc separation of prototypes by .h
and .c files is that the ordering of the functions in the .rst
outout files doesn't make much logical sense.

stracpy is pretty far away from strscpy in the list of functions.

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 16:28 -0600, Jonathan Corbet wrote:
> On Mon, 22 Jul 2019 15:24:33 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > If the functions themselves are fully defined in the .h file, I'd just add
> > > the kerneldoc there as well.  That's how it's usually done, and you want
> > > to keep the documentation and the prototypes together.  
> > 
> > In this case, it's a macro and yes, the kernel-doc could
> > easily be set around the macro in the .h, but my desire
> > is to keep all the string function kernel-doc output
> > together so it should be added to lib/string.c
> > 
> > Are you suggesting I move all the lib/string.c kernel-doc
> > to include/linux/string.h ?
> 
> If you want the *output* together, just put the kernel-doc directives
> together in the RST file that pulls it all in.  Or am I missing something
> here?

Nah, it's me.
I'm not particularly up to date on .rst file usage.

Thanks.

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 16:28 -0600, Jonathan Corbet wrote:
> On Mon, 22 Jul 2019 15:24:33 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > > If the functions themselves are fully defined in the .h file, I'd just add
> > > the kerneldoc there as well.  That's how it's usually done, and you want
> > > to keep the documentation and the prototypes together.  
> > 
> > In this case, it's a macro and yes, the kernel-doc could
> > easily be set around the macro in the .h, but my desire
> > is to keep all the string function kernel-doc output
> > together so it should be added to lib/string.c
> > 
> > Are you suggesting I move all the lib/string.c kernel-doc
> > to include/linux/string.h ?
> 
> If you want the *output* together, just put the kernel-doc directives
> together in the RST file that pulls it all in.  Or am I missing something
> here?

Nah, it's me.
I'm not particularly up to date on .rst file usage.

Thanks.

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Jonathan Corbet]

On Mon, 22 Jul 2019 15:24:33 -0700
Joe Perches <joe@perches.com> wrote:

> > If the functions themselves are fully defined in the .h file, I'd just add
> > the kerneldoc there as well.  That's how it's usually done, and you want
> > to keep the documentation and the prototypes together.  
> 
> In this case, it's a macro and yes, the kernel-doc could
> easily be set around the macro in the .h, but my desire
> is to keep all the string function kernel-doc output
> together so it should be added to lib/string.c
> 
> Are you suggesting I move all the lib/string.c kernel-doc
> to include/linux/string.h ?

If you want the *output* together, just put the kernel-doc directives
together in the RST file that pulls it all in.  Or am I missing something
here?

Thanks,

jon

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 15:57 -0600, Jonathan Corbet wrote:
> On Mon, 22 Jul 2019 14:50:09 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Mon, 2019-07-22 at 23:01 +0200, Stephen Kitt wrote:
> > > How about you submit your current patch set, and I follow up with the above
> > > adapted to stracpy?  
> > 
> > OK, I will shortly after I figure out how to add kernel-doc
> > for stracpy/stracpy_pad to lib/string.c.
> > 
> > It doesn't seem appropriate to add the kernel-doc to string.h
> > as it would be separated from the others in string.c
> > 
> > Anyone got a clue here?  Jonathan?
> 
> If the functions themselves are fully defined in the .h file, I'd just add
> the kerneldoc there as well.  That's how it's usually done, and you want
> to keep the documentation and the prototypes together.

In this case, it's a macro and yes, the kernel-doc could
easily be set around the macro in the .h, but my desire
is to keep all the string function kernel-doc output
together so it should be added to lib/string.c

Are you suggesting I move all the lib/string.c kernel-doc
to include/linux/string.h ?

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 15:57 -0600, Jonathan Corbet wrote:
> On Mon, 22 Jul 2019 14:50:09 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Mon, 2019-07-22 at 23:01 +0200, Stephen Kitt wrote:
> > > How about you submit your current patch set, and I follow up with the above
> > > adapted to stracpy?  
> > 
> > OK, I will shortly after I figure out how to add kernel-doc
> > for stracpy/stracpy_pad to lib/string.c.
> > 
> > It doesn't seem appropriate to add the kernel-doc to string.h
> > as it would be separated from the others in string.c
> > 
> > Anyone got a clue here?  Jonathan?
> 
> If the functions themselves are fully defined in the .h file, I'd just add
> the kerneldoc there as well.  That's how it's usually done, and you want
> to keep the documentation and the prototypes together.

In this case, it's a macro and yes, the kernel-doc could
easily be set around the macro in the .h, but my desire
is to keep all the string function kernel-doc output
together so it should be added to lib/string.c

Are you suggesting I move all the lib/string.c kernel-doc
to include/linux/string.h ?

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Jonathan Corbet]

On Mon, 22 Jul 2019 14:50:09 -0700
Joe Perches <joe@perches.com> wrote:

> On Mon, 2019-07-22 at 23:01 +0200, Stephen Kitt wrote:
> > How about you submit your current patch set, and I follow up with the above
> > adapted to stracpy?  
> 
> OK, I will shortly after I figure out how to add kernel-doc
> for stracpy/stracpy_pad to lib/string.c.
> 
> It doesn't seem appropriate to add the kernel-doc to string.h
> as it would be separated from the others in string.c
> 
> Anyone got a clue here?  Jonathan?

If the functions themselves are fully defined in the .h file, I'd just add
the kerneldoc there as well.  That's how it's usually done, and you want
to keep the documentation and the prototypes together.

jon

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 23:01 +0200, Stephen Kitt wrote:
> How about you submit your current patch set, and I follow up with the above
> adapted to stracpy?

OK, I will shortly after I figure out how to add kernel-doc
for stracpy/stracpy_pad to lib/string.c.

It doesn't seem appropriate to add the kernel-doc to string.h
as it would be separated from the others in string.c

Anyone got a clue here?  Jonathan?

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 23:01 +0200, Stephen Kitt wrote:
> How about you submit your current patch set, and I follow up with the above
> adapted to stracpy?

OK, I will shortly after I figure out how to add kernel-doc
for stracpy/stracpy_pad to lib/string.c.

It doesn't seem appropriate to add the kernel-doc to string.h
as it would be separated from the others in string.c

Anyone got a clue here?  Jonathan?

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 3573 bytes --]

On Mon, 22 Jul 2019 10:59:00 -0700, Joe Perches <joe@perches.com> wrote:
> On Mon, 2019-07-22 at 10:50 -0700, Kees Cook wrote:
> > On Sat, Jul 06, 2019 at 02:42:04PM +0200, Stephen Kitt wrote:  
> > > On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@chromium.org>
> > > wrote:  
> > > > On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:  
> > > > > On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote
> > > > > <nitin.r.gote@intel.com> wrote:    
> > > > > > 1. Deprecate strcpy() in favor of strscpy().    
> > > > > 
> > > > > This isn’t a comment “against” this patch, but something I’ve been
> > > > > wondering recently and which raises a question about how to handle
> > > > > strcpy’s deprecation in particular. There is still one scenario
> > > > > where strcpy is useful: when GCC replaces it with its builtin,
> > > > > inline version...
> > > > > 
> > > > > Would it be worth introducing a macro for
> > > > > strcpy-from-constant-string, which would check that GCC’s builtin
> > > > > is being used (when building with GCC), and fall back to strscpy
> > > > > otherwise?    
> > > > 
> > > > How would you suggest it operate? A separate API, or something like
> > > > the existing overloaded strcpy() macros in string.h?  
> > > 
> > > The latter; in my mind the point is to simplify the thought process for
> > > developers, so strscpy should be the “obvious” choice in all cases,
> > > even when dealing with constant strings in hot paths. Something like
> > > 
> > > __FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t
> > > count) {
> > > 	size_t dest_size = __builtin_object_size(dest, 0);
> > > 	size_t src_size = __builtin_object_size(src, 0);
> > > 	if (__builtin_constant_p(count) &&
> > > 	    __builtin_constant_p(src_size) &&
> > > 	    __builtin_constant_p(dest_size) &&
> > > 	    src_size <= count &&
> > > 	    src_size <= dest_size &&
> > > 	    src[src_size - 1] == '\0') {
> > > 		strcpy(dest, src);
> > > 		return src_size - 1;
> > > 	} else {
> > > 		return __strscpy(dest, src, count);
> > > 	}
> > > }
> > > 
> > > with the current strscpy renamed to __strscpy. I imagine it’s not
> > > necessary to tie this to FORTIFY — __OPTIMIZE__ should be sufficient,
> > > shouldn’t it? Although building on top of the fortified strcpy is
> > > reassuring, and I might be missing something. I’m also not sure how to
> > > deal with the backing strscpy: weak symbol, or something else... At
> > > least there aren’t (yet) any arch-specific implementations of strscpy
> > > to deal with, but obviously they’d still need to be supportable.
> > > 
> > > In my tests, this all gets optimised away, and we end up with code such
> > > as
> > > 
> > > 	strscpy(raead.type, "aead", sizeof(raead.type));
> > > 
> > > being compiled down to
> > > 
> > > 	movl    $1684104545, 4(%rsp)
> > > 
> > > on x86-64, and non-constant code being compiled down to a direct
> > > __strscpy call.  
> > 
> > Thanks for the details! Yeah, that seems nice. I wonder if there is a
> > sensible way to combine these also with the stracpy*() proposal[1], so the
> > call in your example above could just be:
> > 
> > 	stracpy(raead.type, "aead");
> > 
> > (It seems both proposals together would have the correct result...)
> > 
> > [1] https://lkml.kernel.org/r/201907221031.8B87A9DE@keescook  
> 
> Easy enough to do.

How about you submit your current patch set, and I follow up with the above
adapted to stracpy?

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 10:50 -0700, Kees Cook wrote:
> On Sat, Jul 06, 2019 at 02:42:04PM +0200, Stephen Kitt wrote:
> > On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@chromium.org> wrote:
> > > On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:
> > > > On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com>
> > > > wrote:  
> > > > > 1. Deprecate strcpy() in favor of strscpy().  
> > > > 
> > > > This isn’t a comment “against” this patch, but something I’ve been
> > > > wondering recently and which raises a question about how to handle
> > > > strcpy’s deprecation in particular. There is still one scenario where
> > > > strcpy is useful: when GCC replaces it with its builtin, inline version...
> > > > 
> > > > Would it be worth introducing a macro for strcpy-from-constant-string,
> > > > which would check that GCC’s builtin is being used (when building with
> > > > GCC), and fall back to strscpy otherwise?  
> > > 
> > > How would you suggest it operate? A separate API, or something like the
> > > existing overloaded strcpy() macros in string.h?
> > 
> > The latter; in my mind the point is to simplify the thought process for
> > developers, so strscpy should be the “obvious” choice in all cases, even when
> > dealing with constant strings in hot paths. Something like
> > 
> > __FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
> > {
> > 	size_t dest_size = __builtin_object_size(dest, 0);
> > 	size_t src_size = __builtin_object_size(src, 0);
> > 	if (__builtin_constant_p(count) &&
> > 	    __builtin_constant_p(src_size) &&
> > 	    __builtin_constant_p(dest_size) &&
> > 	    src_size <= count &&
> > 	    src_size <= dest_size &&
> > 	    src[src_size - 1] == '\0') {
> > 		strcpy(dest, src);
> > 		return src_size - 1;
> > 	} else {
> > 		return __strscpy(dest, src, count);
> > 	}
> > }
> > 
> > with the current strscpy renamed to __strscpy. I imagine it’s not necessary
> > to tie this to FORTIFY — __OPTIMIZE__ should be sufficient, shouldn’t it?
> > Although building on top of the fortified strcpy is reassuring, and I might
> > be missing something. I’m also not sure how to deal with the backing strscpy:
> > weak symbol, or something else... At least there aren’t (yet) any
> > arch-specific implementations of strscpy to deal with, but obviously they’d
> > still need to be supportable.
> > 
> > In my tests, this all gets optimised away, and we end up with code such as
> > 
> > 	strscpy(raead.type, "aead", sizeof(raead.type));
> > 
> > being compiled down to
> > 
> > 	movl    $1684104545, 4(%rsp)
> > 
> > on x86-64, and non-constant code being compiled down to a direct __strscpy
> > call.
> 
> Thanks for the details! Yeah, that seems nice. I wonder if there is a
> sensible way to combine these also with the stracpy*() proposal[1], so the
> call in your example above could just be:
> 
> 	stracpy(raead.type, "aead");
> 
> (It seems both proposals together would have the correct result...)
> 
> [1] https://lkml.kernel.org/r/201907221031.8B87A9DE@keescook

Easy enough to do.

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Joe Perches]

On Mon, 2019-07-22 at 10:50 -0700, Kees Cook wrote:
> On Sat, Jul 06, 2019 at 02:42:04PM +0200, Stephen Kitt wrote:
> > On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@chromium.org> wrote:
> > > On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:
> > > > On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com>
> > > > wrote:  
> > > > > 1. Deprecate strcpy() in favor of strscpy().  
> > > > 
> > > > This isn’t a comment “against” this patch, but something I’ve been
> > > > wondering recently and which raises a question about how to handle
> > > > strcpy’s deprecation in particular. There is still one scenario where
> > > > strcpy is useful: when GCC replaces it with its builtin, inline version...
> > > > 
> > > > Would it be worth introducing a macro for strcpy-from-constant-string,
> > > > which would check that GCC’s builtin is being used (when building with
> > > > GCC), and fall back to strscpy otherwise?  
> > > 
> > > How would you suggest it operate? A separate API, or something like the
> > > existing overloaded strcpy() macros in string.h?
> > 
> > The latter; in my mind the point is to simplify the thought process for
> > developers, so strscpy should be the “obvious” choice in all cases, even when
> > dealing with constant strings in hot paths. Something like
> > 
> > __FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
> > {
> > 	size_t dest_size = __builtin_object_size(dest, 0);
> > 	size_t src_size = __builtin_object_size(src, 0);
> > 	if (__builtin_constant_p(count) &&
> > 	    __builtin_constant_p(src_size) &&
> > 	    __builtin_constant_p(dest_size) &&
> > 	    src_size <= count &&
> > 	    src_size <= dest_size &&
> > 	    src[src_size - 1] == '\0') {
> > 		strcpy(dest, src);
> > 		return src_size - 1;
> > 	} else {
> > 		return __strscpy(dest, src, count);
> > 	}
> > }
> > 
> > with the current strscpy renamed to __strscpy. I imagine it’s not necessary
> > to tie this to FORTIFY — __OPTIMIZE__ should be sufficient, shouldn’t it?
> > Although building on top of the fortified strcpy is reassuring, and I might
> > be missing something. I’m also not sure how to deal with the backing strscpy:
> > weak symbol, or something else... At least there aren’t (yet) any
> > arch-specific implementations of strscpy to deal with, but obviously they’d
> > still need to be supportable.
> > 
> > In my tests, this all gets optimised away, and we end up with code such as
> > 
> > 	strscpy(raead.type, "aead", sizeof(raead.type));
> > 
> > being compiled down to
> > 
> > 	movl    $1684104545, 4(%rsp)
> > 
> > on x86-64, and non-constant code being compiled down to a direct __strscpy
> > call.
> 
> Thanks for the details! Yeah, that seems nice. I wonder if there is a
> sensible way to combine these also with the stracpy*() proposal[1], so the
> call in your example above could just be:
> 
> 	stracpy(raead.type, "aead");
> 
> (It seems both proposals together would have the correct result...)
> 
> [1] https://lkml.kernel.org/r/201907221031.8B87A9DE@keescook

Easy enough to do.

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Kees Cook]

On Sat, Jul 06, 2019 at 02:42:04PM +0200, Stephen Kitt wrote:
> On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@chromium.org> wrote:
> > On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:
> > > On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com>
> > > wrote:  
> > > > 1. Deprecate strcpy() in favor of strscpy().  
> > > 
> > > This isn’t a comment “against” this patch, but something I’ve been
> > > wondering recently and which raises a question about how to handle
> > > strcpy’s deprecation in particular. There is still one scenario where
> > > strcpy is useful: when GCC replaces it with its builtin, inline version...
> > > 
> > > Would it be worth introducing a macro for strcpy-from-constant-string,
> > > which would check that GCC’s builtin is being used (when building with
> > > GCC), and fall back to strscpy otherwise?  
> > 
> > How would you suggest it operate? A separate API, or something like the
> > existing overloaded strcpy() macros in string.h?
> 
> The latter; in my mind the point is to simplify the thought process for
> developers, so strscpy should be the “obvious” choice in all cases, even when
> dealing with constant strings in hot paths. Something like
> 
> __FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
> {
> 	size_t dest_size = __builtin_object_size(dest, 0);
> 	size_t src_size = __builtin_object_size(src, 0);
> 	if (__builtin_constant_p(count) &&
> 	    __builtin_constant_p(src_size) &&
> 	    __builtin_constant_p(dest_size) &&
> 	    src_size <= count &&
> 	    src_size <= dest_size &&
> 	    src[src_size - 1] == '\0') {
> 		strcpy(dest, src);
> 		return src_size - 1;
> 	} else {
> 		return __strscpy(dest, src, count);
> 	}
> }
> 
> with the current strscpy renamed to __strscpy. I imagine it’s not necessary
> to tie this to FORTIFY — __OPTIMIZE__ should be sufficient, shouldn’t it?
> Although building on top of the fortified strcpy is reassuring, and I might
> be missing something. I’m also not sure how to deal with the backing strscpy:
> weak symbol, or something else... At least there aren’t (yet) any
> arch-specific implementations of strscpy to deal with, but obviously they’d
> still need to be supportable.
> 
> In my tests, this all gets optimised away, and we end up with code such as
> 
> 	strscpy(raead.type, "aead", sizeof(raead.type));
> 
> being compiled down to
> 
> 	movl    $1684104545, 4(%rsp)
> 
> on x86-64, and non-constant code being compiled down to a direct __strscpy
> call.

Thanks for the details! Yeah, that seems nice. I wonder if there is a
sensible way to combine these also with the stracpy*() proposal[1], so the
call in your example above could just be:

	stracpy(raead.type, "aead");

(It seems both proposals together would have the correct result...)

[1] https://lkml.kernel.org/r/201907221031.8B87A9DE@keescook

-- 
Kees Cook

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 2873 bytes --]

On Sat, 6 Jul 2019 14:42:04 +0200, Stephen Kitt <steve@sk2.org> wrote:
> On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@chromium.org> wrote:
> > On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:  
> > > On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com>
> > > wrote:    
> > > > 1. Deprecate strcpy() in favor of strscpy().    
> > > 
> > > This isn’t a comment “against” this patch, but something I’ve been
> > > wondering recently and which raises a question about how to handle
> > > strcpy’s deprecation in particular. There is still one scenario where
> > > strcpy is useful: when GCC replaces it with its builtin, inline
> > > version...
> > > 
> > > Would it be worth introducing a macro for strcpy-from-constant-string,
> > > which would check that GCC’s builtin is being used (when building with
> > > GCC), and fall back to strscpy otherwise?    
> > 
> > How would you suggest it operate? A separate API, or something like the
> > existing overloaded strcpy() macros in string.h?  
> 
> The latter; in my mind the point is to simplify the thought process for
> developers, so strscpy should be the “obvious” choice in all cases, even
> when dealing with constant strings in hot paths. Something like
> 
> __FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
> {
> 	size_t dest_size = __builtin_object_size(dest, 0);
> 	size_t src_size = __builtin_object_size(src, 0);
> 	if (__builtin_constant_p(count) &&
> 	    __builtin_constant_p(src_size) &&
> 	    __builtin_constant_p(dest_size) &&
> 	    src_size <= count &&
> 	    src_size <= dest_size &&
> 	    src[src_size - 1] == '\0') {
> 		strcpy(dest, src);
> 		return src_size - 1;
> 	} else {
> 		return __strscpy(dest, src, count);
> 	}
> }
> 
> with the current strscpy renamed to __strscpy. I imagine it’s not necessary
> to tie this to FORTIFY — __OPTIMIZE__ should be sufficient, shouldn’t it?
> Although building on top of the fortified strcpy is reassuring, and I might
> be missing something. I’m also not sure how to deal with the backing
> strscpy: weak symbol, or something else... At least there aren’t (yet) any
> arch-specific implementations of strscpy to deal with, but obviously they’d
> still need to be supportable.

And there are at least two baked-in assumptions here: src really is a
constant string (so the if should only trigger then), to avoid TOCTTOU races,
and there is only a single null byte at the end of src.

> In my tests, this all gets optimised away, and we end up with code such as
> 
> 	strscpy(raead.type, "aead", sizeof(raead.type));
> 
> being compiled down to
> 
> 	movl    $1684104545, 4(%rsp)
> 
> on x86-64, and non-constant code being compiled down to a direct __strscpy
> call.
> 
> Regards,
> 
> Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

On Tue, 2 Jul 2019 10:25:04 -0700, Kees Cook <keescook@chromium.org> wrote:
> On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:
> > On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com>
> > wrote:  
> > > 1. Deprecate strcpy() in favor of strscpy().  
> > 
> > This isn’t a comment “against” this patch, but something I’ve been
> > wondering recently and which raises a question about how to handle
> > strcpy’s deprecation in particular. There is still one scenario where
> > strcpy is useful: when GCC replaces it with its builtin, inline version...
> > 
> > Would it be worth introducing a macro for strcpy-from-constant-string,
> > which would check that GCC’s builtin is being used (when building with
> > GCC), and fall back to strscpy otherwise?  
> 
> How would you suggest it operate? A separate API, or something like the
> existing overloaded strcpy() macros in string.h?

The latter; in my mind the point is to simplify the thought process for
developers, so strscpy should be the “obvious” choice in all cases, even when
dealing with constant strings in hot paths. Something like

__FORTIFY_INLINE ssize_t strscpy(char *dest, const char *src, size_t count)
{
	size_t dest_size = __builtin_object_size(dest, 0);
	size_t src_size = __builtin_object_size(src, 0);
	if (__builtin_constant_p(count) &&
	    __builtin_constant_p(src_size) &&
	    __builtin_constant_p(dest_size) &&
	    src_size <= count &&
	    src_size <= dest_size &&
	    src[src_size - 1] == '\0') {
		strcpy(dest, src);
		return src_size - 1;
	} else {
		return __strscpy(dest, src, count);
	}
}

with the current strscpy renamed to __strscpy. I imagine it’s not necessary
to tie this to FORTIFY — __OPTIMIZE__ should be sufficient, shouldn’t it?
Although building on top of the fortified strcpy is reassuring, and I might
be missing something. I’m also not sure how to deal with the backing strscpy:
weak symbol, or something else... At least there aren’t (yet) any
arch-specific implementations of strscpy to deal with, but obviously they’d
still need to be supportable.

In my tests, this all gets optimised away, and we end up with code such as

	strscpy(raead.type, "aead", sizeof(raead.type));

being compiled down to

	movl    $1684104545, 4(%rsp)

on x86-64, and non-constant code being compiled down to a direct __strscpy
call.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Kees Cook]

On Mon, Jul 01, 2019 at 08:42:39AM +0000, Gote, Nitin R wrote:
> Hi Kees,
> 
> As per my understanding, I have updated strncpy() section in Documentation/process/deprecated.rst for strscpy_pad() case. Other two cases of strncpy() are already explained. 
> 
> Also updated checkpatch for __nonstring case.
> 
> Could you please give your inputs on below diff changes ? If this looks good, I will send the patch.
> 
> Diff changes :
> 
> diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
> index 49e0f64..6ab05ac 100644
> --- a/Documentation/process/deprecated.rst
> +++ b/Documentation/process/deprecated.rst
> @@ -102,6 +102,9 @@ still be used, but destinations should be marked with the `__nonstring
>  <https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
>  attribute to avoid future compiler warnings.
> 
> +If a caller is using NUL-terminated strings, and destination needing
> +trailing NUL, then the safe replace is :c:func:`strscpy_pad()`.

I'd move this above the __nonstring discussion and remove the memset
mention. How about doing this?

diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 49e0f64a3427..f564de3caf76 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
+:c:func:`strscpy`. If, however, the destination buffer still needs
+NUL-padding, the safe replacement is :c:func:`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

> +
>  strlcpy()
>  ---------
>  :c:func:`strlcpy` reads the entire source buffer first, possibly exceeding
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 342c7c7..d3c0587 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -595,6 +595,10 @@ our %deprecated_apis = (
>         "rcu_barrier_sched"                     => "rcu_barrier",
>         "get_state_synchronize_sched"           => "get_state_synchronize_rcu",
>         "cond_synchronize_sched"                => "cond_synchronize_rcu",
> +       "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",

I found the "Or" strange here; I think just "or" is fine.

-Kees

>  );
> 
> Thanks and Regards,
> Nitin Gote
> 
> -----Original Message-----
> From: Kees Cook [mailto:keescook@chromium.org] 
> Sent: Friday, June 28, 2019 8:16 PM
> To: Gote, Nitin R <nitin.r.gote@intel.com>
> Cc: jannh@google.com; kernel-hardening@lists.openwall.com
> Subject: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().
> 
> On Fri, Jun 28, 2019 at 05:25:48PM +0530, Nitin Gote wrote:
> > Added warnings in checkpatch.pl script to :
> > 
> > 1. Deprecate strcpy() in favor of strscpy().
> > 2. Deprecate strlcpy() in favor of strscpy().
> > 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> > 
> > Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
> 
> Excellent, yes. Can you also add a bit to the strncpy() section in Documentation/process/deprecated.rst so that all three cases of strncpy() are explained:
> 
> - strncpy() into NUL-terminated target should use strscpy()
> - strncpy() into NUL-terminated target needing trailing NUL: strscpy_pad()
> - strncpy() into non-NUL-terminated target should have target marked
>   with __nonstring.
> 
> (and probably mention the __nonstring case in checkpatch too)
> 
> -Kees
> 
> > ---
> >  scripts/checkpatch.pl | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 
> > 342c7c7..bb0fa11 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -595,6 +595,9 @@ our %deprecated_apis = (
> >  	"rcu_barrier_sched"			=> "rcu_barrier",
> >  	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
> >  	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
> > +	"strcpy"				=> "strscpy",
> > +	"strlcpy"				=> "strscpy",
> > +	"strncpy"				=> "strscpy or strscpy_pad",
> >  );
> > 
> >  #Create a search pattern for all these strings to speed up a loop 
> > below
> > --
> > 2.7.4
> > 
> 
> --
> Kees Cook

-- 
Kees Cook

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Kees Cook]

On Sat, Jun 29, 2019 at 06:15:37PM +0200, Stephen Kitt wrote:
> On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com> wrote:
> > 1. Deprecate strcpy() in favor of strscpy().
> 
> This isn’t a comment “against” this patch, but something I’ve been wondering
> recently and which raises a question about how to handle strcpy’s deprecation
> in particular. There is still one scenario where strcpy is useful: when GCC
> replaces it with its builtin, inline version...
> 
> Would it be worth introducing a macro for strcpy-from-constant-string, which
> would check that GCC’s builtin is being used (when building with GCC), and
> fall back to strscpy otherwise?

How would you suggest it operate? A separate API, or something like the
existing overloaded strcpy() macros in string.h?

-- 
Kees Cook

RE: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Gote, Nitin R]

Hi Kees,

As per my understanding, I have updated strncpy() section in Documentation/process/deprecated.rst for strscpy_pad() case. Other two cases of strncpy() are already explained. 

Also updated checkpatch for __nonstring case.

Could you please give your inputs on below diff changes ? If this looks good, I will send the patch.

Diff changes :

diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst
index 49e0f64..6ab05ac 100644
--- a/Documentation/process/deprecated.rst
+++ b/Documentation/process/deprecated.rst
@@ -102,6 +102,9 @@ still be used, but destinations should be marked with the `__nonstring
 <https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html>`_
 attribute to avoid future compiler warnings.

+If a caller is using NUL-terminated strings, and destination needing
+trailing NUL, then the safe replace is :c:func:`strscpy_pad()`.
+
 strlcpy()
 ---------
 :c:func:`strlcpy` reads the entire source buffer first, possibly exceeding
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 342c7c7..d3c0587 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -595,6 +595,10 @@ our %deprecated_apis = (
        "rcu_barrier_sched"                     => "rcu_barrier",
        "get_state_synchronize_sched"           => "get_state_synchronize_rcu",
        "cond_synchronize_sched"                => "cond_synchronize_rcu",
+       "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",
 );

Thanks and Regards,
Nitin Gote

-----Original Message-----
From: Kees Cook [mailto:keescook@chromium.org] 
Sent: Friday, June 28, 2019 8:16 PM
To: Gote, Nitin R <nitin.r.gote@intel.com>
Cc: jannh@google.com; kernel-hardening@lists.openwall.com
Subject: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

On Fri, Jun 28, 2019 at 05:25:48PM +0530, Nitin Gote wrote:
> Added warnings in checkpatch.pl script to :
> 
> 1. Deprecate strcpy() in favor of strscpy().
> 2. Deprecate strlcpy() in favor of strscpy().
> 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> 
> Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>

Excellent, yes. Can you also add a bit to the strncpy() section in Documentation/process/deprecated.rst so that all three cases of strncpy() are explained:

- strncpy() into NUL-terminated target should use strscpy()
- strncpy() into NUL-terminated target needing trailing NUL: strscpy_pad()
- strncpy() into non-NUL-terminated target should have target marked
  with __nonstring.

(and probably mention the __nonstring case in checkpatch too)

-Kees

> ---
>  scripts/checkpatch.pl | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 
> 342c7c7..bb0fa11 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -595,6 +595,9 @@ our %deprecated_apis = (
>  	"rcu_barrier_sched"			=> "rcu_barrier",
>  	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
>  	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
> +	"strcpy"				=> "strscpy",
> +	"strlcpy"				=> "strscpy",
> +	"strncpy"				=> "strscpy or strscpy_pad",
>  );
> 
>  #Create a search pattern for all these strings to speed up a loop 
> below
> --
> 2.7.4
> 

--
Kees Cook

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On Fri, 28 Jun 2019 17:25:48 +0530, Nitin Gote <nitin.r.gote@intel.com> wrote:
> 1. Deprecate strcpy() in favor of strscpy().

This isn’t a comment “against” this patch, but something I’ve been wondering
recently and which raises a question about how to handle strcpy’s deprecation
in particular. There is still one scenario where strcpy is useful: when GCC
replaces it with its builtin, inline version...

Would it be worth introducing a macro for strcpy-from-constant-string, which
would check that GCC’s builtin is being used (when building with GCC), and
fall back to strscpy otherwise?

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] checkpatch: Added warnings in favor of strscpy().

[Kees Cook]

On Fri, Jun 28, 2019 at 05:25:48PM +0530, Nitin Gote wrote:
> Added warnings in checkpatch.pl script to :
> 
> 1. Deprecate strcpy() in favor of strscpy().
> 2. Deprecate strlcpy() in favor of strscpy().
> 3. Deprecate strncpy() in favor of strscpy() or strscpy_pad().
> 
> Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>

Excellent, yes. Can you also add a bit to the strncpy() section in
Documentation/process/deprecated.rst so that all three cases of strncpy()
are explained:

- strncpy() into NUL-terminated target should use strscpy()
- strncpy() into NUL-terminated target needing trailing NUL: strscpy_pad()
- strncpy() into non-NUL-terminated target should have target marked
  with __nonstring.

(and probably mention the __nonstring case in checkpatch too)

-Kees

> ---
>  scripts/checkpatch.pl | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 342c7c7..bb0fa11 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -595,6 +595,9 @@ our %deprecated_apis = (
>  	"rcu_barrier_sched"			=> "rcu_barrier",
>  	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
>  	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
> +	"strcpy"				=> "strscpy",
> +	"strlcpy"				=> "strscpy",
> +	"strncpy"				=> "strscpy or strscpy_pad",
>  );
> 
>  #Create a search pattern for all these strings to speed up a loop below
> --
> 2.7.4
> 

-- 
Kees Cook

[PATCH] checkpatch: Added warnings in favor of strscpy().

[Nitin Gote]

Added warnings in checkpatch.pl script to :

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

Signed-off-by: Nitin Gote <nitin.r.gote@intel.com>
---
 scripts/checkpatch.pl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 342c7c7..bb0fa11 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -595,6 +595,9 @@ our %deprecated_apis = (
 	"rcu_barrier_sched"			=> "rcu_barrier",
 	"get_state_synchronize_sched"		=> "get_state_synchronize_rcu",
 	"cond_synchronize_sched"		=> "cond_synchronize_rcu",
+	"strcpy"				=> "strscpy",
+	"strlcpy"				=> "strscpy",
+	"strncpy"				=> "strscpy or strscpy_pad",
 );

 #Create a search pattern for all these strings to speed up a loop below
--
2.7.4