Re: [PATCH] printk: fix buffer overflow potential for print_text()

[Alexander Gordeev <agordeev@linux.ibm.com>]

On Thu, Jan 14, 2021 at 06:10:12PM +0106, John Ogness wrote:
> Before commit b6cf8b3f3312 ("printk: add lockless ringbuffer"),
> msg_print_text() would only write up to size-1 bytes into the
> provided buffer. Some callers expect this behavior and append
> a terminator to returned string. In particular:
> 
> arch/powerpc/xmon/xmon.c:dump_log_buf()
> arch/um/kernel/kmsg_dump.c:kmsg_dumper_stdout()
> 
> msg_print_text() has been replaced by record_print_text(), which
> currently fills the full size of the buffer. This causes a
> buffer overflow for the above callers.
> 
> Change record_print_text() so that it will only use size-1 bytes
> for text data. Also, for paranoia sakes, add a terminator after
> the text data.
> 
> And finally, document this behavior so that it is clear that only
> size-1 bytes are used and a terminator is added.

Hi John,

I am seeing KASAN reporting incorrect 1-byte access in exactly
same location Sven has identified before. In case there no
fix for it yet, please see below what happens in case of pretty
large buffer - WARN_ONCE() invocation in my case.

> 
> Fixes: b6cf8b3f3312 ("printk: add lockless ringbuffer")
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/printk/printk.c | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ffdd0dc7ec6d..73f9eae19f05 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1293,9 +1293,15 @@ static size_t info_print_prefix(const struct printk_info  *info, bool syslog,
>   *   - Add prefix for each line.
>   *   - Add the trailing newline that has been removed in vprintk_store().
>   *   - Drop truncated lines that do not longer fit into the buffer.
> + *   - Add a trailing newline.
> + *   - Add a string terminator.
> + *
> + * Since the produced string is always terminated, the maximum possible
> + * return value is @r->text_buf_size - 1;
>   *
>   * Return: The length of the updated/prepared text, including the added
> - * prefixes and the newline. The dropped line(s) are not counted.
> + * prefixes and the newline. The terminator is not counted. The dropped
> + * line(s) are not counted.
>   */
>  static size_t record_print_text(struct printk_record *r, bool syslog,
>  				bool time)
> @@ -1338,26 +1344,31 @@ static size_t record_print_text(struct printk_record *r, bool syslog,
>  
>  		/*
>  		 * Truncate the text if there is not enough space to add the
> -		 * prefix and a trailing newline.
> +		 * prefix and a trailing newline and a terminator.
>  		 */
> -		if (len + prefix_len + text_len + 1 > buf_size) {
> +		if (len + prefix_len + text_len + 1 + 1 > buf_size) {
>  			/* Drop even the current line if no space. */
> -			if (len + prefix_len + line_len + 1 > buf_size)
> +			if (len + prefix_len + line_len + 1 + 1 > buf_size)
>  				break;
>  
> -			text_len = buf_size - len - prefix_len - 1;
> +			text_len = buf_size - len - prefix_len - 1 - 1;
>  			truncated = true;
>  		}
>  
>  		memmove(text + prefix_len, text, text_len);
>  		memcpy(text, prefix, prefix_len);
>  
> +		/*
> +		 * Increment the prepared length to include the text and
> +		 * prefix that were just moved+copied. Also increment for the
> +		 * newline at the end of this line. If this is the last line,
> +		 * there is no newline, but it will be added immediately below.
> +		 */
>  		len += prefix_len + line_len + 1;

(1) the next iteration of would-be-length stored in len

> -
>  		if (text_len == line_len) {

(2) but the buffer is processed, so we get here

>  			/*
> -			 * Add the trailing newline removed in
> -			 * vprintk_store().
> +			 * This is the last line. Add the trailing newline
> +			 * removed in vprintk_store().
>  			 */
>  			text[prefix_len + line_len] = '\n';
>  			break;

(3) and bail out from the loop

> @@ -1382,6 +1393,14 @@ static size_t record_print_text(struct printk_record *r, bool syslog,
>  		text_len -= line_len + 1;
>  	}
>  
> +	/*
> +	 * If a buffer was provided, it will be terminated. Space for the
> +	 * string terminator is guaranteed to be available. The terminator is
> +	 * not counted in the return value.
> +	 */
> +	if (buf_size > 0)
> +		text[len] = 0;

(4) trying to terminate, but len is beyond the buffer

BUG: KASAN: global-out-of-bounds in record_print_text+0x1d4/0x248
Write of size 1 at addr 00000000bf9e6992 by task swapper/0/1

Reverting the patch shuts the complain.

> +
>  	return len;
>  }
>  
> -- 
> 2.20.1
>