linux-kernel - Re: [PATCH] printk: ringbuffer: fix line counting

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:   Thu, 14 Jan 2021 14:11:28 +0100
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>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] printk: ringbuffer: fix line counting

On Wed 2021-01-13 15:48:34, John Ogness wrote:
> Counting text lines in a record simply involves counting the number
> of newline characters (+1). However, it is searching the full data
> block for newline characters, even though the text data can be (and
> often is) a subset of that area. Since the extra area in the data
> block was never initialized, the result is that extra newlines may
> be seen and counted.

Great catch!

> Restrict newline searching to the text data length.
> 
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@...utronix.de>

Reviewed-by: Petr Mladek <pmladek@...e.com>

There is a note below.

> ---
>  kernel/printk/printk_ringbuffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk_ringbuffer.c b/kernel/printk/printk_ringbuffer.c
> index 6704f06e0417..8a7b7362c0dd 100644
> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1718,7 +1718,7 @@ static bool copy_data(struct prb_data_ring *data_ring,
>  
>  	/* Caller interested in the line count? */
>  	if (line_count)
> -		*line_count = count_lines(data, data_size);
> +		*line_count = count_lines(data, len);
>  
>  	/* Caller interested in the data content? */
>  	if (!buf || !buf_size)

Another question is what line count should be returned when
the data are copied into the buffer. In this case, the text
might get shrunken even more.

Well, this case is not supported by the API at the moment.
@line_count is defined only in prb_read_valid_info() where
the buffer is always NULL.

But we might add a WARN_ONCE() or a comment there to prevent
similar mistakes in the future.

Best Regards,
Petr