linux-kernel - Re: [PATCH printk v1 1/1] printk: nbcon: Allow unsafe write

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: <aMq80xcRtQbthDiT@pathway.suse.cz>
Date: Wed, 17 Sep 2025 15:51:15 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Breno Leitao <leitao@...ian.org>, Mike Galbraith <efault@....de>,
	linux-kernel@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v1 1/1] printk: nbcon: Allow unsafe write_atomic()
 for panic

On Wed 2025-09-17 14:53:26, John Ogness wrote:
> On 2025-09-16, Petr Mladek <pmladek@...e.com> wrote:
> >> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> >> index 646801813415..8c2966b85ac3 100644
> >> --- a/kernel/printk/nbcon.c
> >> +++ b/kernel/printk/nbcon.c
> >> @@ -972,14 +972,18 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
> >>  	/*
> >>  	 * This function should never be called for consoles that have not
> >>  	 * implemented the necessary callback for writing: i.e. legacy
> >> -	 * consoles and, when atomic, nbcon consoles with no write_atomic().
> >> +	 * consoles and, when atomic, nbcon consoles with no write_atomic()
> >> +	 * or an unsafe write_atomic() without allowing unsafe takeovers.
> >>  	 * Handle it as if ownership was lost and try to continue.
> >>  	 *
> >>  	 * Note that for nbcon consoles the write_thread() callback is
> >>  	 * mandatory and was already checked in nbcon_alloc().
> >>  	 */
> >> -	if (WARN_ON_ONCE((use_atomic && !con->write_atomic) ||
> >> -			 !(console_srcu_read_flags(con) & CON_NBCON))) {
> >> +	if (WARN_ON_ONCE(!(console_srcu_read_flags(con) & CON_NBCON) ||
> >> +			 (use_atomic &&
> >> +			  (!con->write_atomic ||
> >> +			   (!ctxt->allow_unsafe_takeover &&
> >> +			    (console_srcu_read_flags(con) & CON_NBCON_ATOMIC_UNSAFE)))))) {
> >
> > The condition seems to be correct. But it is evil. I wonder whether
> > it would make sense to replace this with:
> >
> > 	flags = console_srcu_read_flags(con);
> >
> > 	if (WARN_ON_ONCE(!(flags & CON_NBCON) ||
> > 			 !console_is_usable(con, flags, use_atomic, ctxt->allow_unsafe_takeover))) {
> >
> >
> > Note that I have added the 4th parameter intentionally, see below.
> 
> ...
> 
> > It would be more reliable when the check was integrated into
> > console_is_usable(). I guess that you did not do it because
> > it added another parameter...
> 
> Not all console_is_usable() call sites have a printing context. That is
> why I only added the checks only to the actual ->write_atomic() paths
> that were possible via nbcon_atomic_flush_unsafe().

I see. But I still believe that it fits well into console_is_usable().
It is similar to the "use_atomic" parameter which depends on the
context as well. We could guess the context most of the time,
so that we hardcode the "use_atomic" value, ...


> > Or maybe, we could define @allow_unsafe_takeover via a global variable,
> > e.g. panic_nbcon_allow_unsafe_takeover. And it might be valid
> > only on the panic CPU, e.g.
> >
> > static inline
> > bool nbcon_allow_unsafe_takeover(void)
> > {
> > 	return panic_on_this_cpu() && panic_nbcon_allow_unsafe_takeover;
> > }
> >
> > It is a kind of hack. But it might be better than the 4th parameter.
> > And it would simplify few other APIs.
> 
> After weighing the pros/cons I think that a global variable makes the
> most sense. It will simplify internal APIs and provide all
> console_is_usable() users a correct value. And the end result is no
> different than what we do now.
> 
> We could also keep its setting inside nbcon_atomic_flush_unsafe() so
> that the variable remains a printk-internal variable.

Sounds good to me.

Best Regards,
Petr