From: Kees Cook <keescook@chromium.org>
To: "Gote, Nitin R" <nitin.r.gote@intel.com>
Cc: "jannh@google.com" <jannh@google.com>,
"kernel-hardening@lists.openwall.com"
<kernel-hardening@lists.openwall.com>
Subject: Re: [PATCH] checkpatch: Added warnings in favor of strscpy().
Date: Tue, 2 Jul 2019 10:31:33 -0700 [thread overview]
Message-ID: <201907021025.C798E540@keescook> (raw)
In-Reply-To: <12356C813DFF6F479B608F81178A561586C2AC@BGSMSX101.gar.corp.intel.com>
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
next prev parent reply other threads:[~2019-07-02 21:04 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-28 11:55 [PATCH] checkpatch: Added warnings in favor of strscpy() Nitin Gote
2019-06-28 14:46 ` Kees Cook
2019-07-01 8:42 ` Gote, Nitin R
2019-07-02 17:31 ` Kees Cook [this message]
2019-06-29 16:15 ` Stephen Kitt
2019-07-02 17:25 ` Kees Cook
2019-07-06 12:42 ` Stephen Kitt
2019-07-07 7:40 ` Stephen Kitt
2019-07-22 17:50 ` Kees Cook
2019-07-22 17:59 ` Joe Perches
2019-07-22 21:01 ` Stephen Kitt
2019-07-22 21:50 ` Joe Perches
2019-07-22 21:57 ` Jonathan Corbet
2019-07-22 22:24 ` Joe Perches
2019-07-22 22:28 ` Jonathan Corbet
2019-07-22 22:35 ` Joe Perches
2019-07-24 11:41 ` Joe Perches
2019-07-04 5:54 Nitin Gote
2019-07-04 20:46 ` Joe Perches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201907021025.C798E540@keescook \
--to=keescook@chromium.org \
--cc=jannh@google.com \
--cc=kernel-hardening@lists.openwall.com \
--cc=nitin.r.gote@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).