linux-kernel - Re: [PATCH next 1/2] printk: avoid and/or handle record truncation

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200929115107.GO6442@alley>
Date:   Tue, 29 Sep 2020 13:51:07 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH next 1/2] printk: avoid and/or handle record truncation

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@...utronix.de>
> Reported-by: Marek Szyprowski <m.szyprowski@...sung.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