linux-kernel - Re: [PATCH] vt: Fix sleeping functions called from atomic context

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: <5810790.S3fJg7CHgZ@localhost.localdomain>
Date:   Tue, 16 Nov 2021 18:28:28 +0100
From:   "Fabio M. De Francesco" <fmdefrancesco@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Jiri Slaby <jirislaby@...nel.org>,
        Max Filippov <jcmvbkbc@...il.com>,
        David Sterba <dsterba@...e.com>,
        Bhaskar Chowdhury <unixbhaskar@...il.com>,
        Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>,
        Igor Matheus Andrade Torrente <igormtorrente@...il.com>,
        nick black <dankamongmen@...il.com>,
        linux-kernel@...r.kernel.org,
        syzbot+5f47a8cea6a12b77a876@...kaller.appspotmail.com,
        Marco Elver <elver@...gle.com>
Subject: Re: [PATCH] vt: Fix sleeping functions called from atomic context

On Tuesday, November 16, 2021 5:59:24 PM CET Greg Kroah-Hartman wrote:
> On Tue, Nov 16, 2021 at 04:35:07PM +0100, Fabio M. De Francesco wrote:
> > On Tuesday, November 16, 2021 3:58:44 PM CET Greg Kroah-Hartman wrote:
> > > On Tue, Nov 16, 2021 at 03:49:37PM +0100, Fabio M. De Francesco wrote:
> > > > Fix two sleeping functions called from atomic context by doing 
immediate
> > > > return to the caller if !preemptible() evaluates 'true'. Remove two
> > > > in_interrupt() tests because they are not suited for being used here.
> > > > 
> > > > Since functions do_con_write() and con_flush_chars() might sleep in
> > > > console_lock(), it must be assured that they are never executed in
> > > > atomic contexts.
> > > > 
> > > > This issue is reported by Syzbot which notices that they are executed
> > > > while holding spinlocks and with interrupts disabled. Actually Syzbot
> > > > emits a first report and then, after fixing do_con_write(), a second
> > > > report for the same problem in con_flush_chars() because these 
functions
> > > > are called one after the other by con_write().
> > > > 
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Reported-by: syzbot+5f47a8cea6a12b77a876@...kaller.appspotmail.com
> > > > Suggested-by: Marco Elver <elver@...gle.com>
> > > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@...il.com>
> > > > ---
> > > >  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..508f8a56d361 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 (!preemptible())
> > > >  		return count;
> > > 
> > > Very odd, what code is calling these functions to trigger this check?
> > 
> > This is the call trace reported by Syzbot (https://syzkaller.appspot.com/
bug?
> > id=fe5a4d5a2482bd73064db5de5d28e024f1e2a387):
> > 
> > Call Trace:
> >  <TASK>
> >  __dump_stack lib/dump_stack.c:88 [inline]
> >  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
> >  __might_resched.cold+0x222/0x26b kernel/sched/core.c:9539
> >  console_lock+0x17/0x80 kernel/printk/printk.c:2522
> >  do_con_write+0x10f/0x1e40 drivers/tty/vt/vt.c:2908
> >  con_write+0x21/0x40 drivers/tty/vt/vt.c:3295
> >  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
> >  n_tty_ioctl_helper+0x299/0x2d0 drivers/tty/tty_ioctl.c:880
> > 
> > 	^^^^^^^^^^
> > n_tty_ioctl_helper() disabled interrupts via spin_lock_irq(&tty-
>flow.lock).
> > 
> >  n_hdlc_tty_ioctl+0xd2/0x340 drivers/tty/n_hdlc.c:633
> >  tty_ioctl+0xc69/0x1670 drivers/tty/tty_io.c:2814
> >  vfs_ioctl fs/ioctl.c:51 [inline]
> >  __do_sys_ioctl fs/ioctl.c:874 [inline]
> >  __se_sys_ioctl fs/ioctl.c:860 [inline]
> >  __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:860
> >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > 
> > > Shouldn't the caller be fixed instead?
> > 
> > Maybe that the caller has no need to disable IRQs, but I cannot yet 
answer to 
> > this particular question.
> > 
> > > What changed to suddenly cause this to show up?
> > 
> > Commit c545b66c6922 ("tty: Serialize tcflow() with other tty flow control 
> > changes") introduced a call to spin_lock_irq() for command "TCOON", just 
> > before calling __start_tty().
> 
> That commit happened in 2014.  Why is this suddenly an issue now that no
> one ever saw before?
> 
> I am worried you are not actually fixing the real issue here by just
> making syzbot be quiet.  Can you work to figure out what the real issue
> is please?

Yes, Greg. For sure I'll try my best to figure out what the real issue is.

No doubts that this code has been around for very long time without no one 
that had noticed anything wrong, so this report looks weird. There must be 
something that I still can't see. Therefore, I agree with you that, with an 
high degree of probability, the real issue is somewhere else. (if there is a 
real one).

It looks interesting to investigate, although for doing it properly I'll need 
some time because I know very little of this code.

Thanks for your reply,

Fabio M. De Francesco 

> 
> thanks,
> 
> greg k-h
>