linux-kernel - Re: [PATCH] printk/nmi: avoid direct printk()-s from __printk_nmi

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:   Tue, 30 Aug 2016 11:04:36 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] printk/nmi: avoid direct printk()-s from
 __printk_nmi_flush()

On Tue 2016-08-30 16:58:34, Sergey Senozhatsky wrote:
> Petr,
> one more question. Not related to the patch, but still related to NMI.
> 
> can NMI nest?

AFAIK, they cannot. NMIs should be disabled until iret is called.
Therefore we should be on the safe side if iret is not called
inside the NMI handler. But this should not happen because
it would cause other problems, like using wrong return address.

Well, x86 nmi code has some hacks to handle exceptions inside
NMI handlers that use iret. But printk_nmi_enter()/printk_nmi_exit()
are never nested there. It is prevented by the nmi_state per-CPU
variable. See do_nmi() in arch/x86/kernel/nmi.c.


> shouldn't we do something like this then? /* not tested */
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> 
> ---
>  kernel/printk/internal.h | 2 ++
>  kernel/printk/nmi.c      | 9 ++++++++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 7fd2838..5b7508f 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -31,6 +31,8 @@ extern raw_spinlock_t logbuf_lock;
>   * via per-CPU variable.
>   */
>  DECLARE_PER_CPU(printk_func_t, printk_func);
> +DECLARE_PER_CPU(printk_func_t, printk_func_saved);
> +
>  static inline __printf(1, 0) int vprintk_func(const char *fmt, va_list args)
>  {
>  	return this_cpu_read(printk_func)(fmt, args);
> diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> index 16bab47..9d83929 100644
> --- a/kernel/printk/nmi.c
> +++ b/kernel/printk/nmi.c
> @@ -39,6 +39,7 @@
>   * were handled or when IRQs are blocked.
>   */
>  DEFINE_PER_CPU(printk_func_t, printk_func) = vprintk_default;
> +DEFINE_PER_CPU(printk_func_t, printk_func_saved);
>  static int printk_nmi_irq_ready;
>  atomic_t nmi_message_lost;
>  
> @@ -259,10 +260,16 @@ void __init printk_nmi_init(void)
>  
>  void printk_nmi_enter(void)
>  {
> +	printk_func_t func = this_cpu_read(printk_func);
> +
> +	if (func != vprintk_nmi)
> +		this_cpu_write(printk_func_saved, func);
>  	this_cpu_write(printk_func, vprintk_nmi);
>  }
>  
>  void printk_nmi_exit(void)
>  {
> -	this_cpu_write(printk_func, vprintk_default);
> +	printk_func_t func = this_cpu_read(printk_func_saved);
> +
> +	this_cpu_write(printk_func, func);

This would handle only one level of nesting. If nesting was possible
we would probably need something else. Fortunately, I believe that we
do not need this.

Thanks for checking the code.

Best Regards,
Petr