Message-ID: <CAK8P3a1d-DnF58zZqxMYE3DrtZO9Bs3a2o-cuvTupnJeTeG2ww@mail.gmail.com>
Date:   Thu, 1 Mar 2018 10:57:49 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     David Rientjes <rientjes@...gle.com>,
        Ingo Molnar <mingo@...nel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        "Lendacky, Thomas" <thomas.lendacky@....com>,
        Will Deacon <will.deacon@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Willy Tarreau <w@....eu>,
        Xiongfeng Wang <wangxiongfeng2@...wei.com>,
        Martin Sebor <msebor@...il.com>
Subject: Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)

On Mon, Feb 19, 2018 at 1:01 AM, Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
> On Mon, Feb 19, 2018 at 12:20 AM, David Rientjes <rientjes@...gle.com> wrote:
>> On Sat, 17 Feb 2018, Miguel Ojeda wrote:
>>
>>> From the GCC manual:
>>>
>>> The nonstring variable attribute specifies that an object or member
>>> declaration with type array of char or pointer to char is intended to
>>> store character arrays that do not necessarily contain a terminating NUL
>>> character. This is useful in detecting uses of such arrays or pointers
>>> with functions that expect NUL-terminated strings, and to avoid warnings
>>> when such an array or pointer is used as an argument to a bounded string
>>> manipulation function such as strncpy.
>>>
>>>   https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html
>>>
>>> Some reports are already coming to the LKML regarding these
>>> warnings. When they are false positives, we can use __nonstring to let
>>> gcc know a NUL character is not required; like in this case:
>>>
>>>   https://lkml.org/lkml/2018/1/16/135
>>>
>>> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
>>> Cc: Ingo Molnar <mingo@...nel.org>
>>> Cc: Josh Poimboeuf <jpoimboe@...hat.com>
>>> Cc: Kees Cook <keescook@...omium.org>
>>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>>> Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
>>> Cc: Will Deacon <will.deacon@....com>
>>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>>> Cc: David Rientjes <rientjes@...gle.com>
>>
>> I would have expected to have seen __nonstring used somewhere as part of
>> this patch.
>
> Do you mean to expand the commit message with an actual code example
> instead of the links to the docs and the discussion about the report?
> Otherwise, if you mean in the actual commit, I think in that case it
> should be a patch series, not a single commit.
>
> In any case, the key point here is to agree on the short-term policy:
> i.e. whether we want to disable the upcoming warning or try to take
> advantage of it (which not *necessarily* implies using __nonstring,
> there are other workarounds; though where applicable, __nonstring is
> probably the right thing to use).

What David was asking for is to have a couple of users of the
__nonstring attribute in places for which it is the right solution.

I would suggest making it a patch series, with patch 1/x introducing
the attribute (i.e. your patch), and followed by additional patches
that add the attribute to individual header files or drivers for which
it is the right solution.

When I looked at the warning, I found that we have around 120 file
for which we warn. The majority of them are actually questionable
uses of strncpy() that probably should have been strscpy(), but
most of those do not actually cause undefined behavior.

A smaller number like the example from ext4 are nonstrings
(i.e. character arrays without nul-termination) that would benefit
from the nonstring attribute. About half of those are actually
arrays of u8/__u8/uint8_t/__uint8_t for which the currently
implemented nonstring attribute is invalid, and it seems odd
to convert those to 'char', e.g.

struct ext4_super_block {
        __le32  s_first_error_time;     /* first time an error happened */
        __le32  s_first_error_ino;      /* inode involved in first error */
        __le64  s_first_error_block;    /* block involved of first error */
-       __u8    s_first_error_func[32]; /* function where the error happened */
+       char    s_first_error_func[32] __nonstring;     /* function
where the error happened */
        __le32  s_first_error_line;     /* line number where error happened */
        __le32  s_last_error_time;      /* most recent time of an error */
        __le32  s_last_error_ino;       /* inode involved in last error */
        __le32  s_last_error_line;      /* line number where error happened */
        __le64  s_last_error_block;     /* block involved of last error */
-       __u8    s_last_error_func[32];  /* function where the error happened */
+       char    s_last_error_func[32] __nonstring;      /* function
where the error happened */

doesn't feel right. Maybe we can extend gcc to also accept
the attribute on arrays of other 8-bit types.

> [By the way, CC'ing Xiongfeng, Willy and Arnd, since they were
> involved in the example report; sorry guys!].

Martin Sebor also asked me about this, he's the one that worked on
the gcc code that introduced the warning. Sorry for not replying earlier.

For a complete list of affected files, see https://pastebin.com/eWFQf58i
this is what I come up with by doing randconfig builds, but I have not
tried to submit additional patches here, since I'm sure that a lot of
those are wrong -- they need a much closer inspection to decide which
ones are actual bugs vs harmless warnings, and which ones should
use strscpy()/strlcpy() vs a nonstring annotation or a rewrite of that
function.

       Arnd

Powered by blists - more mailing lists