linux-kernel - Re: [PATCH printk v4 8/8] printk: nbcon: Allow drivers to mark unsafe regions and check state

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: <ZQQX6V93nJGoBwfJ@alley>
Date:   Fri, 15 Sep 2023 10:38:01 +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 8/8] printk: nbcon: Allow drivers to mark
 unsafe regions and check state

On Fri 2023-09-08 20:56:08, John Ogness wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> For the write_atomic callback, the console driver may have unsafe
> regions that need to be appropriately marked. Provide functions
> that accept the nbcon_write_context struct to allow for the driver
> to enter and exit unsafe regions.
> 
> Also provide a function for drivers to check if they are still the
> owner of the console.
> 
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index e2c274f4142e..04fac73c6e96 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -660,6 +660,32 @@ static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_s
>  	return false;
>  }
>  
> +/**
> + * nbcon_can_proceed - Check whether ownership can proceed
> + * @wctxt:	The write context that was handed to the write function
> + *
> + * Return:	True if this context still owns the console. False if
> + *		ownership was handed over or taken.
> + *
> + * Must be invoked at appropriate safe places in the driver.

This is a bit vague. I guess that enter_unsafe()/exit_unsafe() will be
used most of the time. The guestion is if this need to be called
in another locations explicitely.

I would write something similar as I suggested for nbcon_context_can_proceed():

  * It is used in nbcon_enter_unsafe() to make sure that it still owns the lock.
  * Also it is used in nbcon_exit_unsafe() to eventually free the lock
  * for a higher priority context which asked for the friendly handover.
  *
  * It can be called inside an unsafe section when the console is just
  * temporary in safe state instead of exiting and entering the unsafe
  * state.
  *
  * Also it can be called in the safe context before doing an expensive
  * safe operation. It does not make sense to do the operation when
  * a higher priority context took the lock.

> + *
> + * When this function returns false then the calling context no longer owns
> + * the console and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context.
> + */
> +bool nbcon_can_proceed(struct nbcon_write_context *wctxt)
> +{
> +	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +	struct console *con = ctxt->console;
> +	struct nbcon_state cur;
> +
> +	nbcon_state_read(con, &cur);
> +
> +	return nbcon_context_can_proceed(ctxt, &cur);
> +}
> +EXPORT_SYMBOL_GPL(nbcon_can_proceed);
> +

With the updated comment:

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

Best Regards,
Petr