linux-kernel - Re: [PATCH V2] auxdisplay: use correct string length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANiq72n0nvCPcR5t=z2dxdP-pJC+QsMmhAN7NXX-+HS-LhN8BA@mail.gmail.com>
Date:   Mon, 12 Feb 2018 21:42:23 +0100
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Willy Tarreau <w@....eu>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Xiongfeng Wang <wangxiongfeng2@...wei.com>,
        Dan <dan.carpenter@...cle.com>, Arnd Bergmann <arnd@...db.de>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2] auxdisplay: use correct string length

On Mon, Feb 12, 2018 at 6:11 PM, Willy Tarreau <w@....eu> wrote:
> On Mon, Feb 12, 2018 at 01:53:57PM +0100, Miguel Ojeda wrote:
>> > diff --git a/drivers/auxdisplay/panel.c b/drivers/auxdisplay/panel.c
>> > index ea7869c..d288900 100644
>> > --- a/drivers/auxdisplay/panel.c
>> > +++ b/drivers/auxdisplay/panel.c
>> > @@ -1506,10 +1506,10 @@ static struct logical_input *panel_bind_key(const char *name, const char *press,
>> >         key->rise_time = 1;
>> >         key->fall_time = 1;
>> >
>> > -       strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str));
>> > -       strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str));
>> > +       strncpy(key->u.kbd.press_str, press, sizeof(key->u.kbd.press_str) - 1);
>> > +       strncpy(key->u.kbd.repeat_str, repeat, sizeof(key->u.kbd.repeat_str) - 1);
>> >         strncpy(key->u.kbd.release_str, release,
>> > -               sizeof(key->u.kbd.release_str));
>> > +               sizeof(key->u.kbd.release_str) - 1);
>>
>> Are you sure about this patch? `kbd` says "strings can be non null-terminated".
>>
>> Willy, maybe those should just be memcpy()s? (unless the remaining
>> bytes, if any, must be 0).
>
> For me this seems to be the result of yet another very stupid gcc warning
> trying to dissuade us from using well defined fonctions... it's unimaginable
> how gcc warnings have become stupid and irrelevant since its developers
> stopped using C to write it :-(
>
> If you want to work around this wrong warning, probably that increasing the
> destination storage size by one and adding -1 to strncpy() would shut it up

It does indeed.

> but that really becomes quite annoying to have to modify code and storage
> just to shut down a dumbass compiler trying to be smart.
>

I have looked a bit deeper at this new warning. It comes with -Wall,
and to trigger it the compiler needs to have some optimization passes
enabled (-O2 works). See https://godbolt.org/g/dydPah to play with it
in action.

Assuming gcc 8 comes out with this warning implemented as of today
(likely), we have several options then:

  a) -Wno-stringop-truncation for gcc >= 8. Note: the warning checks
for several kinds of potential issues, some might be useful.

  b) Use the new __attribute__((nonstring)) for gcc >= 8. For
instance, the kbd strings are anyhow marked with the /* strings can be
non null-terminated */ comment, so we might just as well replace the
comment with the attribute.

  c) For this case: +1 the array length, -1 the sizeof; as Willy
suggested; or similar workarounds.

  d) For this case: try to make gcc see that the source is a small
enough array. Unlikely, due to the keypad_profile indirection and the
loop in keypad_init.

Since anyway we will be hit by this warning in N places in the kernel
now or in the future, let's ask Linus et. al. about what should be the
policy to follow :)

Miguel

> Willy