linux-kernel - Re: [PATCH 1/3] printk: Fix kdb_trap

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: <20171005133844.GA16068@pathway.suse.cz>
Date:   Thu, 5 Oct 2017 15:38:44 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     sergey.senozhatsky@...il.com, linux-kernel@...r.kernel.org,
        rostedt@...dmis.org, mingo@...nel.org, tglx@...utronix.de,
        Jason Wessel <jason.wessel@...driver.com>
Subject: Re: [PATCH 1/3] printk: Fix kdb_trap_printk placement

On Thu 2017-09-28 14:18:24, Peter Zijlstra wrote:
> Some people figured vprintk_emit() makes for a nice API and exported
> it, bypassing the kdb trap.
> 
> This still leaves vprintk_nmi() outside of the kbd reach, should that
> be fixed too?
> 
> Cc: Jason Wessel <jason.wessel@...driver.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> ---
>  kernel/printk/printk.c |   18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1811,6 +1811,11 @@ asmlinkage int vprintk_emit(int facility
>  	int printed_len;
>  	bool in_sched = false;
>  
> +#ifdef CONFIG_KGDB_KDB
> +	if (unlikely(kdb_trap_printk && kdb_printf_cpu < 0))
> +		return vkdb_printf(KDB_MSGSRC_PRINTK, fmt, args);
> +#endif

Hmm, this will get called also from scheduler and timer code
via printk_deferred(). I am afraid that it is not safe.

If I get it correctly, vkdb_printf() might call printk()
when kdb_printf_cpu are set to a real CPU number. Then
we will fall through and try to call consoles.

Fortunately, I think that we do not need this patch at all.
vkdb_printf() is called here only when kdb_trap_printk is set.
It is used the following way:

static void kdb_dumpregs(struct pt_regs *regs)
{
[...]
	kdb_trap_printk++;
	show_regs(regs);
	kdb_trap_printk--;
[...]
}

or 

static int kdb_ftdump(int argc, const char **argv)
{
[...]
	kdb_trap_printk++;
	ftrace_dump_buf(skip_lines, cpu_file);
	kdb_trap_printk--;
[...]
}

It looks like a nasty hack to reuse an existing code
that calls printk(). The aim is to get the output
of these printk's on the kdb console instead of
the log buffer and other consoles.

Note that kdb_dumpregs(), kdb_ftdump() implement
kdb commands that might be called from the kdb console.

If these commands are always called from normal context
then we do not need to care of NMI and other special printk
variants.

Or can the kdb console commands be called in NMI context?

Best Regards,
Petr