Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 29 Sep 2020 13:51:07 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
        Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH next 1/2] printk: avoid and/or handle record truncation
Message-ID: <20200929115107.GO6442@alley>
References: <20200926015526.8921-1-john.ogness@linutronix.de>
 <20200926015526.8921-2-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200926015526.8921-2-john.ogness@linutronix.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Sat 2020-09-26 04:01:25, John Ogness wrote:
> If a reader provides a buffer that is smaller than the message text,
> the @text_len field of @info will have a value larger than the buffer
> size. If readers blindly read @text_len bytes of data without
> checking the size, they will read beyond their buffer.

Great catch!

> Add this check to record_print_text() to properly recognize when such
> truncation needs to occur.
> 
> Add a maximum size argument to the ringbuffer function to extend
> records so that records can not be created that are larger than the
> buffer size of readers.
> 
> When extending records (LOG_CONT), do not extend records beyond
> LOG_LINE_MAX since that is the maximum size available in the buffers
> used by consoles and syslog.
> 
> Fixes: f5f022e53b87 ("printk: reimplement log_cont using record extension")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
>  kernel/printk/printk.c            |  7 ++++++-
>  kernel/printk/printk_ringbuffer.c | 12 ++++++++++--
>  kernel/printk/printk_ringbuffer.h |  2 +-
>  3 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 78f68b4830dc..270f19b60e6f 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1357,6 +1357,11 @@ static size_t record_print_text(struct printk_record *r, bool syslog,
>  	size_t len = 0;
>  	char *next;
>  
> +	if (text_len > buf_size) {
> +		text_len = buf_size;
> +		truncated = true;

@truncate must not be set here. Otherwise, the prefix would not be
added when there no '\n' in the entire string. It would call:

			/* Drop truncated line(s). */
			if (truncated)
				break;

before copying the prefix.

It is enough to remove the line. It will be set in the very first
cycle anyway. We need to add one prefix at all. It would require to
truncate even more bytes.

Otherwise, the patch looks good to me.

Best Regards,
Petr