linux-kernel - Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN

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]

Date:   Fri, 16 Jun 2017 14:01:57 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Alexander Potapenko <glider@...gle.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Networking <netdev@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Arend van Spriel <arend.vanspriel@...adcom.com>,
        Jiri Slaby <jslaby@...e.com>,
        Samuel Thibault <samuel.thibault@...-lyon.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [PATCH v2 03/11] tty: kbd: reduce stack size with KASAN

On Thu, Jun 15, 2017 at 6:53 AM, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
> On Thu, Jun 15, 2017 at 06:52:21AM +0200, Greg Kroah-Hartman wrote:
>> On Wed, Jun 14, 2017 at 11:15:38PM +0200, Arnd Bergmann wrote:
>> > As reported by kernelci, some functions in the VT code use significant
>> > amounts of kernel stack when local variables get inlined into the caller
>> > multiple times:
>> >
>> > drivers/tty/vt/keyboard.c: In function 'kbd_keycode':
>> > drivers/tty/vt/keyboard.c:1452:1: error: the frame size of 2240 bytes is larger than 2048 bytes [-Werror=frame-larger-than=]
>> >
>> > Annotating those functions as noinline_if_stackbloat prevents the inlining
>> > and reduces the overall stack usage in this driver.
>> >
>> > Signed-off-by: Arnd Bergmann <arnd@...db.de>
>> > ---
>> >  drivers/tty/vt/keyboard.c | 6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
>> > index f4166263bb3a..c0d111444a0e 100644
>> > --- a/drivers/tty/vt/keyboard.c
>> > +++ b/drivers/tty/vt/keyboard.c
>> > @@ -301,13 +301,13 @@ int kbd_rate(struct kbd_repeat *rpt)
>> >  /*
>> >   * Helper Functions.
>> >   */
>> > -static void put_queue(struct vc_data *vc, int ch)
>> > +static noinline_if_stackbloat void put_queue(struct vc_data *vc, int ch)
>> >  {
>> >     tty_insert_flip_char(&vc->port, ch, 0);
>> >     tty_schedule_flip(&vc->port);
>> >  }
>>
>> Ugh, really?  We have to start telling gcc not to be stupid here?
>> That's not going to be easy, and will just entail us doing this all over
>> the place, right?
>>
>> The code isn't asking to be inlined, so why is gcc allowing it to be
>> done that way?  Doesn't that imply gcc is the problem here?
>
> Wait, you are now, in this patch, _asking_ for it to be inlined.  How is
> that solving anything?

The three functions that gain the attribute are all those that gcc decided
to inline for itself. Usually gcc makes reasonable inlining decisions, so
I left the existing behavior my marking them as 'inline' without
CONFIG_KASAN and 'noinline' when KASAN is enabled.

Would you rather see this patch instead?

diff --git a/include/linux/tty_flip.h b/include/linux/tty_flip.h
index c28dd523f96e..25348c5ffcb7 100644
--- a/include/linux/tty_flip.h
+++ b/include/linux/tty_flip.h
@@ -13,8 +13,8 @@ extern int tty_prepare_flip_string(struct tty_port *port,
 extern void tty_flip_buffer_push(struct tty_port *port);
 void tty_schedule_flip(struct tty_port *port);

-static inline int tty_insert_flip_char(struct tty_port *port,
-                                       unsigned char ch, char flag)
+static noinline_if_stackbloat int
+tty_insert_flip_char(struct tty_port *port, unsigned char ch, char flag)
 {
        struct tty_buffer *tb = port->buf.tail;
        int change;

This is just as good at eliminating the crazy stack usage in vt/keyboard.o,
but it will also impact all other users of that function.

        Arnd