linux-kernel - Re: [PATCHv2 4/4] printk: make sure we always print console disabled message

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190523065947.GB18333@jagdpanzerIV>
Date:   Thu, 23 May 2019 15:59:47 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [PATCHv2 4/4] printk: make sure we always print console disabled
 message

On (05/15/19 16:47), Petr Mladek wrote:
> On Fri 2019-04-26 14:44:45, Sergey Senozhatsky wrote:
> > 
> > Forgot to mention that the series is still in RFC phase.
> > 
> > 
> > On (04/26/19 14:33), Sergey Senozhatsky wrote:
> > [..]
> > > +++ b/kernel/printk/printk.c
> > > @@ -2613,6 +2613,12 @@ static int __unregister_console(struct console *console)
> > >  	pr_info("%sconsole [%s%d] disabled\n",
> > >  		(console->flags & CON_BOOT) ? "boot" : "",
> > >  		console->name, console->index);
> > > +	/*
> > > +	 * Print 'console disabled' on all the consoles, including the
> > > +	 * one we are about to unregister.
> > > +	 */
> > > +	console_unlock();
> > > +	console_lock();
> > >  
> > >  	res = _braille_unregister_console(console);
> > >  	if (res)
> > 
> > Need to think more if this is race free...
> 
> I am afraid that it is racy against for_each_console() when
> removing the boot consoles.

Can you explain? Do you mean that we can execute two paths unregistering
the same bcon?

	CPU0					CPU1

	console_lock();
	__unregister_console(bconA)		console_lock();
	console_unlock();

						__unregister_console(bconA);
						for (a = console_drivers->next ...)
							if (a == console)
								unregister();
							// console bconA is
							// not in the list
							// anymore
						console_unlock();

	for (a = console_drivers->next ...)
		if (a == console)
	console_unlock();


This CPU0 will never see bconA in the console drivers list.
But... it will try to do

	console->flags &= ~CON_ENABLED;

Which we need to fix.

Do not disable console which is not on the console_drivers list.

---
index 1177ea4b3fe1..e729992cb4e4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2660,7 +2660,8 @@ static int __unregister_console(struct console *console)
 	if (console_drivers != NULL && console->flags & CON_CONSDEV)
 		console_drivers->flags |= CON_CONSDEV;
 
-	console->flags &= ~CON_ENABLED;
+	if (!res)
+		console->flags &= ~CON_ENABLED;
 	return res;
 }
 
---
	-ss