Message-ID: <d76825e1-6e0f-a75c-21b0-3501dbc9d7a9@gmail.com>
Date:   Mon, 5 Mar 2018 12:05:22 -0700
From:   Martin Sebor <msebor@...il.com>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
        Arnd Bergmann <arnd@...db.de>
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>
Subject: Re: [PATCH] Support the nonstring variable attribute (gcc >= 8)

On 03/02/2018 10:36 AM, Miguel Ojeda wrote:
> On Thu, Mar 1, 2018 at 10:57 AM, Arnd Bergmann <arnd@...db.de> wrote:
>> 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 understood :-) My question was regarding where he was asking to see it.
>
>> 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.
>
> Yep, that is what I suggested too.
>
>>
>> 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.
>
> Then it looks like enabling the warning by default is useful and not
> too noisy (at least for just char).
>
>>
>> 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.
>
> Hum... On one hand, the warning is meant to protect against misuses of
> the typical string handling functions, and those take pointers to
> char. Therefore, one could argue that using signed or unsigned char
> already marks an array/pointer as "not a string" (for the purposes of
> the attribute).
>
> On the other hand, people *will* call string handling functions with
> signed and unsigned char, and for those cases, it is useful to have
> the warning nevertheless and being able to annotate those arrays with
> nonstring, which is also good documentation-wise. On top of that, C
> specifies char as equivalent to either signed or unsigned char (even
> if it is a distinct type), so one could argue it should work for the
> three types anyway.
>
> Given that 1) this is a warning that can disabled just fine and that
> 2) we already have real life cases using nonstring, non-char arrays
> calling typical string handling functions, I would favor supporting
> the warning and the attribute for all the three 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.
>>
>
> Maybe you can pass this to him? (maybe open a bug in gcc's bugzilla?)

I've opened bug 84725 to extend attribute nonstring to the other
two character types:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84725

Thanks
Martin

>
>> 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.
>
> Indeed -- nice work anyway finding those. If we agree on getting the
> nonstring attribute, maybe you can send that patch as an RFC to ping
> the respective maintainers?
>
> Thanks,
> Miguel
>

Powered by blists - more mailing lists