From: Julia Lawall <julia.lawall@lip6.fr>
To: Dominique Martinet <asmadeus@codewreck.org>
Cc: "Masahiro Yamada" <yamada.masahiro@socionext.com>,
"Ville Syrjälä" <ville.syrjala@linux.intel.com>,
"Gilles Muller" <Gilles.Muller@lip6.fr>,
"Nicolas Palix" <nicolas.palix@imag.fr>,
"Michal Marek" <michal.lkml@markovi.net>,
cocci@systeme.lip6.fr, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] coccinelle: suggest replacing strncpy+truncation by strscpy
Date: Fri, 20 Jul 2018 07:33:42 +0200 (CEST) [thread overview]
Message-ID: <alpine.DEB.2.20.1807200731240.2349@hadrien> (raw)
In-Reply-To: <1532047018-23754-1-git-send-email-asmadeus@codewreck.org>
[-- Attachment #1: Type: text/plain, Size: 4733 bytes --]
On Fri, 20 Jul 2018, Dominique Martinet wrote:
> Using strscpy instead of strncpy+truncation is simpler and fixes part
> of the following class of new gcc warnings:
>
> drivers/gpu/drm/i915/intel_tv.c: In function ‘intel_tv_get_modes’:
> drivers/gpu/drm/i915/intel_tv.c:1358:3: error: ‘strncpy’ specified bound 32 equals
> destination size [-Werror=stringop-truncation]
> strncpy(mode_ptr->name, input->name, DRM_DISPLAY_MODE_LEN);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Note that this is not a proper fix for this warning. The warning was
> intended to have developers check the return code of strncpy and
> act in case of truncation (print a warning, abort the function or
> something similar if the original string was not nul terminated);
> the change to strscpy only works because gcc does not handle the
> function the same way.
>
> A previous version of this patch suggested to use strlcpy instead,
> but strscpy is preferred because it does not read more than the given
> length of the source string unlike strlcpy, which could read after the
> end of the buffer in case of unterminated string.
>
> strscpy does however not clear the end of the destination buffer, so
> there is a risk of information leak if the full buffer is copied as is
> out of the kernel - this needs manual checking.
As fasr as I can tell from lkml, only one of these patches has been
accepted? There was also a concern about an information leak that there
was no response to. Actually, I would prefer that more of the generated
patches are accepted before accepting the semantic patch, for something
that is not quite so obviously correct.
julia
>
> Signed-off-by: Dominique Martinet <asmadeus@codewreck.org>
> ---
> v2:
> - Use strscpy instead of strlcpy, as strlcpy can read after the number
> of requested bytes in the source string, and none of the replaced users
> want to know the source string size length
> - Add longer semantic patch information, warning in particular for
> information leak
> - Lowered Confidence level to medium because of the possibility of
> information leak, that needs manual checking
> - Fix spacing of the diff section and removed unused virtual context
>
> v3:
> - Add license/copyright
> - Rewording of commit message
>
> I didn't see many other remarks, but kept SUGGESTION as discussed.
> I didn't move all virtuals in a single line because none of the other
> kernel patch do it, and still do not see any advantage of moving the
> string to not use a variable so kept that as well.
>
> This should hopefully be the last version :)
>
> .../coccinelle/misc/strncpy_truncation.cocci | 52 +++++++++++++++++++
> 1 file changed, 52 insertions(+)
> create mode 100644 scripts/coccinelle/misc/strncpy_truncation.cocci
>
> diff --git a/scripts/coccinelle/misc/strncpy_truncation.cocci b/scripts/coccinelle/misc/strncpy_truncation.cocci
> new file mode 100644
> index 000000000000..7732cde23a85
> --- /dev/null
> +++ b/scripts/coccinelle/misc/strncpy_truncation.cocci
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/// Use strscpy rather than strncpy(dest,..,sz) + dest[sz-1] = '\0'
> +///
> +//# This makes an effort to find occurences of strncpy followed by forced
> +//# truncation, which can generate gcc stringop-truncation warnings when
> +//# source and destination buffers are the same size.
> +//# Using strscpy will always do that nul-termination for us and not read
> +//# more than the maximum bytes requested in src, use that instead.
> +//#
> +//# The result needs checking that the destination buffer does not need
> +//# its tail zeroed (either cleared beforehand or will never leave the
> +//# kernel) so as not to leak information
> +//
> +// Confidence: Medium
> +// Copyright: (C) 2018 Dominique Martinet
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual patch
> +virtual report
> +virtual org
> +
> +@r@
> +expression dest, src, sz;
> +position p;
> +@@
> +
> +strncpy@p(dest, src, sz);
> +dest[sz - 1] = '\0';
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +msg = "strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
> +cocci.print_main(msg, p)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg = "SUGGESTION: strncpy followed by truncation can be strscpy, if the destination buffer does not need to be fully overwritten"
> +coccilib.report.print_report(p[0], msg)
> +
> +@ok depends on patch@
> +expression r.dest, r.src, r.sz;
> +@@
> +
> +- strncpy
> ++ strscpy
> + (dest, src, sz);
> +- dest[sz - 1] = '\0';
> --
> 2.17.1
>
>
next prev parent reply other threads:[~2018-07-20 5:33 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-13 1:14 [PATCH 01/18] coccinelle: change strncpy+truncation to strlcpy Dominique Martinet
2018-07-13 1:25 ` [PATCH 02/18] block/aoenet: " Dominique Martinet
2018-07-13 14:16 ` Jens Axboe
2018-07-13 15:31 ` Dominique Martinet
2018-07-13 1:25 ` [PATCH 03/18] drm_property: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 04/18] nouveau: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 05/18] iio: " Dominique Martinet
2018-07-15 10:39 ` Jonathan Cameron
2018-07-16 11:42 ` Dominique Martinet
2018-07-22 8:13 ` Jonathan Cameron
2018-07-13 1:25 ` [PATCH 06/18] mptctl: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 07/18] hisilicon: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 08/18] myricom: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 09/18] qlogic/qed: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 10/18] brcmsmac: " Dominique Martinet
2018-07-13 7:19 ` Arend van Spriel
2018-07-13 1:25 ` [PATCH 11/18] wireless/ti: " Dominique Martinet
2018-07-13 7:38 ` Greg Kroah-Hartman
2018-07-13 7:47 ` Arend van Spriel
2018-07-13 8:13 ` Dominique Martinet
2018-07-13 18:56 ` Rustad, Mark D
2018-07-27 9:19 ` Kalle Valo
2018-07-13 1:25 ` [PATCH 12/18] test_power: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 13/18] ibmvscsi: " Dominique Martinet
2018-07-13 1:25 ` [PATCH 14/18] kdb_support: " Dominique Martinet
2018-07-13 10:33 ` Daniel Thompson
2018-07-13 15:18 ` Dominique Martinet
2018-07-16 8:23 ` Daniel Thompson
2018-07-13 1:26 ` [PATCH 15/18] blktrace: " Dominique Martinet
2019-03-15 1:37 ` Steven Rostedt
2019-03-15 2:01 ` Jens Axboe
2019-03-15 6:30 ` Dominique Martinet
2019-03-15 14:29 ` Jens Axboe
2018-07-13 1:26 ` [PATCH 16/18] tools/accounting: " Dominique Martinet
2018-07-13 1:26 ` [PATCH 17/18] perf: " Dominique Martinet
2018-07-13 1:26 ` [PATCH 18/18] cpupower: " Dominique Martinet
2018-07-24 16:31 ` Shuah Khan
2018-08-14 15:45 ` Daniel Díaz
2018-08-14 19:27 ` Dominique Martinet
2018-08-20 14:27 ` Shuah Khan
2018-07-13 7:44 ` [Cocci] [PATCH 01/18] coccinelle: " Himanshu Jha
2018-07-13 8:00 ` Dominique Martinet
2018-07-13 9:14 ` Himanshu Jha
2018-07-13 9:44 ` Julia Lawall
2018-07-13 10:21 ` Himanshu Jha
2018-07-13 10:50 ` Julia Lawall
2018-07-13 16:11 ` Dominique Martinet
2018-07-14 8:12 ` [PATCH v2] coccinelle: strncpy+truncation by strscpy Dominique Martinet
2018-07-14 11:54 ` Julia Lawall
[not found] ` <alpine.DEB.2.20.1807140743550.3356@hadrien>
2018-07-14 13:08 ` Dominique Martinet
2018-07-14 20:36 ` Julia Lawall
2018-07-14 14:34 ` [v2] Coccinelle: Replace strncpy() + truncation by strscpy() SF Markus Elfring
2018-07-20 0:36 ` [PATCH v3] coccinelle: suggest replacing strncpy+truncation by strscpy Dominique Martinet
2018-07-20 5:33 ` Julia Lawall [this message]
2018-07-20 5:40 ` Dominique Martinet
2018-07-20 5:49 ` Julia Lawall
2018-07-20 5:57 ` Dominique Martinet
2018-07-20 6:03 ` Julia Lawall
2018-07-20 11:00 ` [v3] Coccinelle: " SF Markus Elfring
2018-07-20 9:40 ` SF Markus Elfring
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=alpine.DEB.2.20.1807200731240.2349@hadrien \
--to=julia.lawall@lip6.fr \
--cc=Gilles.Muller@lip6.fr \
--cc=asmadeus@codewreck.org \
--cc=cocci@systeme.lip6.fr \
--cc=linux-kernel@vger.kernel.org \
--cc=michal.lkml@markovi.net \
--cc=nicolas.palix@imag.fr \
--cc=ville.syrjala@linux.intel.com \
--cc=yamada.masahiro@socionext.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).