From: Dominique Martinet <asmadeus@codewreck.org> To: Masahiro Yamada <yamada.masahiro@socionext.com> Cc: "Dominique Martinet" <asmadeus@codewreck.org>, "Ville Syrjälä" <ville.syrjala@linux.intel.com>, "Julia Lawall" <Julia.Lawall@lip6.fr>, "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: [PATCH v3] coccinelle: suggest replacing strncpy+truncation by strscpy Date: Fri, 20 Jul 2018 02:36:58 +0200 [thread overview] Message-ID: <1532047018-23754-1-git-send-email-asmadeus@codewreck.org> (raw) In-Reply-To: <1531555951-9627-1-git-send-email-asmadeus@codewreck.org> 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. 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
WARNING: multiple messages have this Message-ID (diff)
From: asmadeus@codewreck.org (Dominique Martinet) To: cocci@systeme.lip6.fr Subject: [Cocci] [PATCH v3] coccinelle: suggest replacing strncpy+truncation by strscpy Date: Fri, 20 Jul 2018 02:36:58 +0200 [thread overview] Message-ID: <1532047018-23754-1-git-send-email-asmadeus@codewreck.org> (raw) In-Reply-To: <1531555951-9627-1-git-send-email-asmadeus@codewreck.org> 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. 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 + + at r@ +expression dest, src, sz; +position p; +@@ + +strncpy at p(dest, src, sz); +dest[sz - 1] = '\0'; + + at 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) + + at 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 0:37 UTC|newest] Thread overview: 100+ 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:14 ` [Cocci] " Dominique Martinet 2018-07-13 1:25 ` [PATCH 02/18] block/aoenet: " Dominique Martinet 2018-07-13 1:25 ` Dominique Martinet 2018-07-13 1:25 ` Dominique Martinet 2018-07-13 12:17 ` Ed Cashin 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 ` Dominique Martinet 2018-07-13 1:25 ` [PATCH 04/18] nouveau: " Dominique Martinet 2018-07-13 1:25 ` 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-22 8:13 ` Jonathan Cameron 2018-07-13 1:25 ` [PATCH 06/18] mptctl: " Dominique Martinet 2018-07-13 1:25 ` 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 ` Dominique Martinet 2018-07-13 1:25 ` [PATCH 13/18] ibmvscsi: " Dominique Martinet 2018-07-13 1:25 ` Dominique Martinet 2018-07-13 1:25 ` 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 2018-07-13 1:26 ` 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-13 1:26 ` 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 7:44 ` Himanshu Jha 2018-07-13 8:00 ` Dominique Martinet 2018-07-13 8:00 ` Dominique Martinet 2018-07-13 9:14 ` Himanshu Jha 2018-07-13 9:14 ` Himanshu Jha 2018-07-13 9:44 ` Julia Lawall 2018-07-13 9:44 ` Julia Lawall 2018-07-13 10:21 ` Himanshu Jha 2018-07-13 10:21 ` Himanshu Jha 2018-07-13 10:50 ` Julia Lawall 2018-07-13 10:50 ` Julia Lawall 2018-07-13 16:11 ` Dominique Martinet 2018-07-13 16:11 ` Dominique Martinet [not found] ` <5e93dba5-1a57-ee59-e714-17a80b3fb031@users.sourceforge.net> 2018-07-13 16:42 ` [Cocci] Coccinelle: " Himanshu Jha [not found] ` <d6fac368-bb43-f53c-0f58-b25a81156a4e@users.sourceforge.net> 2018-07-13 18:41 ` [Cocci] Coccinelle: Development challenges around software profiling Himanshu Jha [not found] ` <536adc42-1680-e4be-bfee-7d01c37a239d@users.sourceforge.net> 2018-07-13 20:26 ` Himanshu Jha [not found] ` <a35ae0ee-13d2-7ac8-99a4-488069983154@users.sourceforge.net> 2018-07-14 9:16 ` [Cocci] [01/18] coccinelle: change strncpy+truncation to strlcpy Dominique Martinet 2018-07-14 11:41 ` Julia Lawall [not found] ` <4b9986b2-957a-081a-038e-afc5acf0bfdd@users.sourceforge.net> 2018-07-13 15:25 ` [Cocci] Coccinelle: " Dominique Martinet 2018-07-13 15:28 ` Julia Lawall 2018-07-14 8:12 ` [PATCH v2] coccinelle: strncpy+truncation by strscpy Dominique Martinet 2018-07-14 8:12 ` [Cocci] " Dominique Martinet 2018-07-14 11:54 ` Julia Lawall 2018-07-14 11:54 ` [Cocci] " Julia Lawall [not found] ` <alpine.DEB.2.20.1807140743550.3356@hadrien> 2018-07-14 13:08 ` Dominique Martinet 2018-07-14 13:08 ` [Cocci] " Dominique Martinet 2018-07-14 20:36 ` Julia Lawall 2018-07-14 20:36 ` [Cocci] " Julia Lawall 2018-07-14 14:34 ` [v2] Coccinelle: Replace strncpy() + truncation by strscpy() SF Markus Elfring 2018-07-14 14:34 ` SF Markus Elfring 2018-07-20 0:36 ` Dominique Martinet [this message] 2018-07-20 0:36 ` [Cocci] [PATCH v3] coccinelle: suggest replacing strncpy+truncation by strscpy Dominique Martinet 2018-07-20 5:33 ` Julia Lawall 2018-07-20 5:33 ` [Cocci] " Julia Lawall 2018-07-20 5:40 ` Dominique Martinet 2018-07-20 5:40 ` [Cocci] " Dominique Martinet 2018-07-20 5:49 ` Julia Lawall 2018-07-20 5:49 ` [Cocci] " Julia Lawall 2018-07-20 5:57 ` Dominique Martinet 2018-07-20 5:57 ` [Cocci] " Dominique Martinet 2018-07-20 6:03 ` Julia Lawall 2018-07-20 6:03 ` [Cocci] " Julia Lawall 2018-07-20 11:00 ` [v3] Coccinelle: " SF Markus Elfring 2018-07-20 11:00 ` SF Markus Elfring 2018-07-20 9:40 ` 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=1532047018-23754-1-git-send-email-asmadeus@codewreck.org \ --to=asmadeus@codewreck.org \ --cc=Gilles.Muller@lip6.fr \ --cc=Julia.Lawall@lip6.fr \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.