linux-kernel - Re: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled

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: <CAHk-=wjVL_CLm-+=7qf2obF6f8D+ujysmqp5dKdAb7UEyo1cZg@mail.gmail.com>
Date:   Wed, 1 Dec 2021 11:05:09 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Peter Hurley <peter@...leysoftware.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled

On Wed, Dec 1, 2021 at 5:41 AM Tetsuo Handa
<penguin-kernel@...ove.sakura.ne.jp> wrote:
>
> What do you think? Can we apply this?

I think this patch is only papering over the problem, and the issue goes deeper.

It may be that "papering over the issue" successfully hides it
completely, but it's still a horribly bad approach.

> > -     if (in_interrupt())
> > +     if (in_interrupt() || irqs_disabled())
> >               return count;

This kind of stuff is broken. Pretty much always.

And in this case, it's still broken, because things like "called under
a non-irq spinlock" would still not show up.

And no, I do *not* mean that the code should try to figure that out. I
mean that the problem goes further up, and that the fact that we get
to do_con_write() in the first place when we're in an invalid context
is wrong, wrong, wrong.

How the heck do we get here from just an ioctl?

Looking at the backtrace, I see

   n_hdlc_send_frames+0x24b/0x490 drivers/tty/n_hdlc.c:290
   tty_wakeup+0xe1/0x120 drivers/tty/tty_io.c:534
   __start_tty drivers/tty/tty_io.c:806 [inline]
   __start_tty+0xfb/0x130 drivers/tty/tty_io.c:799

and apparently it's that hdlc line discipline (and
n_hdlc_send_frames() in particular) that is the problem here.

I think that's where the fix should be.

                Linus