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.ogn...@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
>
