linux-kernel - Re: possible deadlock in console

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:   Thu, 7 Jun 2018 23:01:00 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        syzbot <syzbot+43e93968b964e369db0b@...kaller.appspotmail.com>,
        linux-kernel@...r.kernel.org, rostedt@...dmis.org,
        sergey.senozhatsky@...il.com, syzkaller-bugs@...glegroups.com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>, linux-serial@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: possible deadlock in console_unlock

On (06/07/18 13:00), Petr Mladek wrote:
> > IOW
> > 
> >     tty ioctl
> >     tty_port->lock		IRQ
> >     printk			uart_port->lock
> >     console_owner
> >     uart_port->lock		tty_port->rlock
> 
> Great analyze!

Thanks!

> I am just afraid that there are many other locations like this.

Yep, agree. That's why I suggested the printk_safe context for
most critically important locks.

> > Another way could be - switch to printk_safe mode around that
> > kmalloc():
> > 
> > 	__printk_safe_enter();
> > 	kmalloc(sizeof(struct tty_buffer) + 2 * size, GFP_ATOMIC);
> > 	__printk_safe_exit();
> > 
> > Or, may be, we even can switch to printk_safe mode every time we grab
> > tty_port lock.
>  
> > Perhaps something like this should be done for uart_port->lock
> > as well. Because, technically, we can have the following
> 
> Yeah, we would need this basically around any lock that can be taken
> from console write() callbacks. Well, this would be needed even
> around locks that might be in a chain with a lock used in these
> callbacks (as shown by this report).

Yep. So the plan for now is to wrap the tty_port->lock. Pretty much
an automatic conversion.

Then to convert [may be some for now on] uart_port->lock. Once again,
pretty much can be done a script.

Afterwards just sit down and be humbl^W^W wait for new reports. Then
move those newly discovered unsafe locks under printk_safe context.

Basically, the same macros as we use for logbuf lock in printk.c

A bit of a lazy approach. Can't think of anything better.

I think it's finally the time to start dealing with these
"external" locks, it's been a while.

> BTW: printk_safe context might be too strict. In fact,
> printk_deferred() would be enough. We might think about
> introducing also printk_deferred context.

Could be.
The good thing about printk_safe is that printk_safe sections can nest.
I suspect there might be locks/printk_safe sections nesting at some
point. In any case, switching to a new flavor of printk_safe will be
pretty easy - just replace printk_safe_enter() with printk_foo_enter()
and the same for printk_save_exit().

I'll wait for some time, to see what people will say.
I guess we also need to check if Linus is OK with the proposed solution.

	-ss