Date:   Fri, 20 Jul 2018 02:36:58 +0200
From:   Dominique Martinet <asmadeus@...ewreck.org>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Dominique Martinet <asmadeus@...ewreck.org>,
        Ville Syrjälä 
        <ville.syrjala@...ux.intel.com>,
        Julia Lawall <Julia.Lawall@...6.fr>,
        Gilles Muller <Gilles.Muller@...6.fr>,
        Nicolas Palix <nicolas.palix@...g.fr>,
        Michal Marek <michal.lkml@...kovi.net>, cocci@...teme.lip6.fr,
        linux-kernel@...r.kernel.org
Subject: [PATCH v3] coccinelle: suggest replacing strncpy+truncation by strscpy

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@...ewreck.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';
+
+@...ipt: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)
+
+@...ipt: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

Powered by blists - more mailing lists