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: <3add7ade-9c9b-2839-5a0c-0a38c4be0e34@i-love.sakura.ne.jp>
Date:   Wed, 1 Dec 2021 22:40:38 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Peter Hurley <peter@...leysoftware.com>,
        Jiri Slaby <jirislaby@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] tty: vt: make do_con_write() no-op if IRQ is disabled

What do you think? Can we apply this?

On 2021/11/19 23:55, Tetsuo Handa wrote:
> syzbot is reporting that an unprivileged user who logged in from tty
> console can crash the system using a reproducer shown below [1], for
> commit f9e053dcfc02b0ad ("tty: Serialize tty flow control changes with
> flow_lock") changed __start_tty() from schedulable context to atomic
> context without realizing that drivers/tty/n_hdlc.c is calling
> do_con_write() (which waits for console semaphore) from __start_tty().
> 
> ----------
>   #include <sys/ioctl.h>
>   #include <unistd.h>
> 
>   int main(int argc, char *argv[])
>   {
>     const int disc = 0xd;
> 
>     ioctl(1, TIOCSETD, &disc);
>     while (1) {
>       ioctl(1, TCXONC, 0);
>       write(1, "", 1);
>       ioctl(1, TCXONC, 1); /* Kernel panic - not syncing: scheduling while atomic */
>     }
>   }
> ----------
> 
> While we would need to bring __start_tty() back to schedulable context
> in order to be able to call do_con_write() from ioctl(TCXONC, TCOON),
> changes required might be complicated; an oversight will result in a
> new sleep from atomic context bug or a new race window bug. Therefore,
> firstly let's fix crash problem by protecting do_con_write() side.
> 
> Link: https://syzkaller.appspot.com/bug?extid=5f47a8cea6a12b77a876 [1]
> Reported-by: syzbot <syzbot+5f47a8cea6a12b77a876@...kaller.appspotmail.com>
> Analyzed-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Fixes: f9e053dcfc02b0ad ("tty: Serialize tty flow control changes with flow_lock")
> ---
>  drivers/tty/vt/vt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index 7359c3e80d63..efa16f51c978 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -2902,7 +2902,7 @@ static int do_con_write(struct tty_struct *tty, const unsigned char *buf, int co
>  	struct vt_notifier_param param;
>  	bool rescan;
>  
> -	if (in_interrupt())
> +	if (in_interrupt() || irqs_disabled())
>  		return count;
>  
>  	console_lock();
> @@ -3358,7 +3358,7 @@ static void con_flush_chars(struct tty_struct *tty)
>  {
>  	struct vc_data *vc;
>  
> -	if (in_interrupt())	/* from flush_to_ldisc */
> +	if (in_interrupt() || irqs_disabled())	/* from flush_to_ldisc */
>  		return;
>  
>  	/* if we race with con_close(), vt may be null */
>