linux-kernel - Re: [RFC][PATCH 2/2] printk: take console_sem when accessing console drivers list

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:   Wed, 24 Apr 2019 17:13:06 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [RFC][PATCH 2/2] printk: take console_sem when accessing console
 drivers list

On Tue 2019-04-23 15:25:11, Sergey Senozhatsky wrote:
> We need to take console_sem lock when we iterate console
> drivers list. Otherwise, another CPU can concurrently
> modify console drivers list or console drivers.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> ---
>  kernel/printk/printk.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index b0e361ca1bea..c2bccf58d03e 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2674,12 +2674,17 @@ void register_console(struct console *newcon)
>  	struct console_cmdline *c;
>  	static bool has_preferred;
>  
> -	if (console_drivers)
> -		for_each_console(bcon)
> -			if (WARN(bcon == newcon,
> -					"console '%s%d' already registered\n",
> -					bcon->name, bcon->index))
> -				return;
> +	console_lock();
> +	if (console_drivers) {
> +		for_each_console(bcon) {
> +			if (bcon != newcon)
> +				continue;
> +			WARN(1, "console '%s%d' already registered\n",
> +			     bcon->name, bcon->index);
> +			console_unlock();
> +			return;
> +		}
> +	}
>  
>  	/*
>  	 * before we register a new CON_BOOT console, make sure we don't
> @@ -2691,6 +2696,7 @@ void register_console(struct console *newcon)
>  			if (!(bcon->flags & CON_BOOT)) {
>  				pr_info("Too late to register bootconsole %s%d\n",
>  					newcon->name, newcon->index);
> +				console_unlock();
>  				return;
>  			}
>  		}
> @@ -2701,6 +2707,7 @@ void register_console(struct console *newcon)
>  
>  	if (!has_preferred || bcon || !console_drivers)
>  		has_preferred = preferred_console >= 0;
> +	console_unlock();

We should keep it until the console is added into the list. Otherwise
there are races with accessing the static has_preferred and
the global preferred_console variables.

Also the value of bcon should stay synchronized until we decide
about replaying the log.

IMHO, the only danger might be when con->match() or con->setup()
would want to take console_lock() as well. I checked few drivers
and they looked safe. But I did not check all of them.

What do you think, please?

Best Regards,
Petr