linux-kernel - Re: [PATCH printk v4 4/8] printk: nbcon: Add buffer management

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, 14 Sep 2023 16:55:40 +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>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH printk v4 4/8] printk: nbcon: Add buffer management

On Fri 2023-09-08 20:56:04, John Ogness wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> In case of hostile takeovers it must be ensured that the previous
> owner cannot scribble over the output buffer of the emergency/panic
> context. This is achieved by:
> 
>  - Adding a global output buffer instance for the panic context.
>    This is the only situation where hostile takeovers can occur and
>    there is always at most 1 panic context.
> 
>  - Allocating an output buffer per non-boot console upon console
>    registration. This buffer is used by the console owner when not
>    in panic context. (For boot consoles, the existing shared global
>    legacy output buffer is used instead. Boot console printing will
>    be synchronized with legacy console printing.)
> 
>  - Choosing the appropriate buffer is handled in the acquire/release
>    functions.
> 
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -20,6 +21,9 @@
>   * region and aborts the operation if it detects a takeover.
>   *
>   * In case of panic the nesting context can take over the console forcefully.
> + * If the interrupted context touches the assigned record buffer after
> + * takeover, it does not cause harm because the interrupting single panic
> + * context is assigned its own panic record buffer.
>   *
>   * A concurrent writer on a different CPU with a higher priority can directly
>   * take over if the console is not in an unsafe state by carefully writing

The above chunk had a merge conflict with my proposed changes for
the 2nd patch. It uses a completely different text from the commit
message. I added the info into the new text the following way:

@@ -73,6 +74,10 @@
  *      the console is an unsafe state. It is used only in panic() by
  *      the final attempt to flush consoles in a try and hope mode.
  *
+ *      Note that separate record buffers are used in panic(). As a result,
+ *      the messages can be read and formatted without any risk even after
+ *      using the hostile takeover in unsafe state.
+ *
  * The release function simply clears the 'prio' and 'cpu' fields.
  *
  * All operations on @console::nbcon_state are atomic cmpxchg based


> @@ -426,6 +431,12 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
>  
>  	nbcon_context_acquire_hostile(ctxt, &cur);
>  success:
> +	/* Assign the appropriate buffer for this context. */
> +	if (atomic_read(&panic_cpu) == cpu)
> +		ctxt->pbufs = &panic_nbcon_pbufs;
> +	else
> +		ctxt->pbufs = con->pbufs;
> +
>  	return true;
>  }

Also the above chunk had conflict with the proposed changes. I
resolbed it the following way:

@@ -496,7 +502,18 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
 
 	err = nbcon_context_try_acquire_hostile(ctxt, &cur);
 out:
-	return !err;
+	if (err)
+		return false;
+
+	/* Acquire succeded */
+
+	/* Assign the appropriate buffer for this context. */
+	if (atomic_read(&panic_cpu) == cpu)
+		ctxt->pbufs = &panic_nbcon_pbufs;
+	else
+		ctxt->pbufs = con->pbufs;
+
+	return true;
 }
 
 static bool nbcon_owner_matches(struct nbcon_state *cur, int expected_cpu,


> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3448,6 +3442,15 @@ void register_console(struct console *newcon)
>  		goto unlock;
>  	}
>  
> +	if (newcon->flags & CON_NBCON) {
> +		/*
> +		 * Ensure the nbcon console buffers can be allocated
> +		 * before modifying any global data.
> +		 */
> +		if (!nbcon_alloc(newcon))
> +			goto unlock;
> +	}
> +
>  	/*
>  	 * See if we want to enable this console driver by default.
>  	 *

We have to call nbcon_free() when try_enable_*_console() failed.
Something like:

@@ -3484,8 +3484,10 @@ void register_console(struct console *newcon)
 		err = try_enable_preferred_console(newcon, false);
 
 	/* printk() messages are not printed to the Braille console. */
-	if (err || newcon->flags & CON_BRL)
+	if (err || newcon->flags & CON_BRL) {
+		nbcon_free(newcon);
 		goto unlock;
+	}
 
 	/*
 	 * If we have a bootconsole, and are switching to a real console,


With the proposed changes:

Reviewed-by: Petr Mladek <pmladek@...e.com>

Best Regards,
Petr