linux-kernel - Re: [PATCH 28/38] Externalize string buffer for printk

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] [day] [month] [year] [list]

Message-ID: <542AFBF8.1070004@suse.de>
Date:	Tue, 30 Sep 2014 20:52:40 +0200
From:	Hannes Reinecke <hare@...e.de>
To:	Petr Mladek <pmladek@...e.cz>
CC:	James Bottomley <jbottomley@...allels.com>,
	Christoph Hellwig <hch@...radead.org>,
	linux-scsi@...r.kernel.org, Robert Elliott <elliott@...com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 28/38] Externalize string buffer for printk

On 09/30/2014 06:39 PM, Petr Mladek wrote:
> On Mon 29-09-14 13:58:57, Hannes Reinecke wrote:
>> This patch splits off the actual logging from vprintk_emit()
>> into printk_emit_string(), with vprintk_emit() just being a
>> simple wrapper for formatting the message into a
>> static buffer.
>>
>> With that the caller can pass in a local buffer for
>> printk_emit_string() without increasing the overall stack size.
>>
>> Cc: Steven Rostedt <rostedt@...dmis.org>
>> Cc: LKML <linux-kernel@...r.kernel.org>
>
> Adding Andrew into CC here as well.
>
> There is one potential problem, see below.
>
>> Signed-off-by: Hannes Reinecke <hare@...e.de>
>> ---
>>   include/linux/printk.h |  5 +++++
>>   kernel/printk/printk.c | 36 ++++++++++++++++++++++++------------
>>   2 files changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/linux/printk.h b/include/linux/printk.h
>> index d78125f..9639900 100644
>> --- a/include/linux/printk.h
>> +++ b/include/linux/printk.h
>> @@ -130,6 +130,11 @@ int vprintk_emit(int facility, int level,
>>   		 const char *dict, size_t dictlen,
>>   		 const char *fmt, va_list args);
>>
>> +asmlinkage
>> +int printk_emit_string(int facility, int level,
>> +		       const char *dict, size_t dictlen,
>> +		       char *textbuf, size_t text_len);
>> +
>>   asmlinkage __printf(1, 0)
>>   int vprintk(const char *fmt, va_list args);
>>
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index d13675e..303a1fe 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1618,22 +1618,11 @@ asmlinkage int vprintk_emit(int facility, int level,
>>   			    const char *dict, size_t dictlen,
>>   			    const char *fmt, va_list args)
>>   {
>> -	static int recursion_bug;
>>   	static char textbuf[LOG_LINE_MAX];
>>
>>   	char *text = textbuf;
>>   	size_t text_len = 0;
>> -	enum log_flags lflags = 0;
>> -	unsigned long flags;
>> -	int this_cpu;
>> -	int printed_len = 0;
>> -	bool in_sched = false;
>> -	/* cpu currently holding logbuf_lock in this function */
>> -	static volatile unsigned int logbuf_cpu = UINT_MAX;
>>
>>   	if (level == SCHED_MESSAGE_LOGLEVEL) {
>> -		level = -1;
>> -		in_sched = true;
>> -
>>   		/*
>>   		 * The printf needs to come first; we need the syslog
>>   		 * prefix which might be passed-in as a parameter.
>> @@ -1644,6 +1633,24 @@ asmlinkage int vprintk_emit(int facility, int level,
>>
>>   	text_len += vscnprintf(text + text_len,
>>   			       sizeof(textbuf) - text_len, fmt, args);
>> +	return printk_emit_string(facility, level, dict, dictlen,
>> +				  textbuf, text_len);
>> +}
>> +EXPORT_SYMBOL(vprintk_emit);
>> +
>> +asmlinkage int printk_emit_string(int facility, int level,
>> +				  const char *dict, size_t dictlen,
>> +				  char *textbuf, size_t text_len)
>> +{
>> +	static int recursion_bug;
>> +	char *text = textbuf;
>> +	enum log_flags lflags = 0;
>> +	unsigned long flags;
>> +	int this_cpu;
>> +	int printed_len = 0;
>> +	bool in_sched = false;
>> +	/* cpu currently holding logbuf_lock in this function */
>> +	static volatile unsigned int logbuf_cpu = UINT_MAX;
>
> We should make sure that text_len is lower or equal LOG_LINE_MAX.
>
> I am afraid that printk() code is not able to process longer
> lines. For example, syslog_print() does:
>
>         text = kmalloc(LOG_LINE_MAX + PREFIX_MAX, GFP_KERNEL);
>
> Then it calls msg_print_text() that works with entire messages. So,
> any longer message would freeze syslog_print() because it would newer
> fit into the buffer.
>
Ah. Okay, I haven't thought about this. But yes, you are right.

> I guess that there are more locations like this.
>
> Maybe, we should make LOG_LINE_MAX public, so it could be used on the
> other location either to allocate the buffer or to check the size.
>
Well, for starters we should be truncating the buffer to LOG_LINE_MAX in 
printk_emit_string().
And document that it will only handle messages up to that length.

I'll be posting an updated patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@...e.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/