lkml.org 
[lkml]   [2021]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
From
SubjectRFC: printk: kmsg_dump_get_line_nolock() buffer overflow
Date
Hello,

I have discovered that kmsg_dump_get_line_nolock() is not allowed to
fill the full buffer that it is provided. It should leave at least 1
byte free so that callers can append a terminator.

Example from arch/powerpc/xmon/xmon.c:

kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf), &len);
buf[len] = '\0';

This unusual behavior was not noticed and with commit 896fbe20b4e2
("printk: use the lockless ringbuffer") the implementation of
kmsg_dump_get_line_nolock() was changed so that the full buffer can be
filled. This means that the two kmsg_dump_get_line*() users currently
can have a 1-byte buffer overflow.

This unusual kmsg_dump_get_line_nolock() behavior seems to have been
accidentally introduced with commit 3ce9a7c0ac28 ("printk() - restore
prefix/timestamp printing for multi-newline strings"). Indeed, the
whitespace on the line that causes this behavior is not conform, leading
me to think it was a last-minute change or a typo. (The behavior is
caused by the ">=" instead of an expected ">".)

+ if (print_prefix(msg, syslog, NULL) +
+ text_len + 1>= size - len)
+ break;

Perhaps there was some paranoia involved because this same commit also
fixes a buffer overflow in the old implementation:

- if (len + msg->text_len > size)
- return -EINVAL;
- memcpy(text + len, log_text(msg), msg->text_len);
- len += msg->text_len;
- text[len++] = '\n';

Anyways, we currently have this 1-byte buffer overflow in 5.10 and it
needs to be corrected. I see two possibilties.

1. We leave kmsg_dump_get_line_nolock() as it is and modify the callers
to properly terminate their buffer. This is my preferred
approach. The patch would look like this:

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index dcd817ca2edf..9c3faab3a7a5 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3019,7 +3019,7 @@ dump_log_buf(void)

kmsg_dump_rewind_nolock(&dumper);
xmon_start_pagination();
- while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf), &len)) {
+ while (kmsg_dump_get_line_nolock(&dumper, false, buf, sizeof(buf) - 1, &len)) {
buf[len] = '\0';
printf("%s", buf);
}
diff --git a/arch/um/kernel/kmsg_dump.c b/arch/um/kernel/kmsg_dump.c
index e4abac6c9727..acc1b399fbe2 100644
--- a/arch/um/kernel/kmsg_dump.c
+++ b/arch/um/kernel/kmsg_dump.c
@@ -25,7 +25,7 @@ static void kmsg_dumper_stdout(struct kmsg_dumper *dumper,
return;

printf("kmsg_dump:\n");
- while (kmsg_dump_get_line(dumper, true, line, sizeof(line), &len)) {
+ while (kmsg_dump_get_line(dumper, true, line, sizeof(line) - 1, &len)) {
line[len] = '\0';
printf("%s", line);
}
2. We modify kmsg_dump_get_line_nolock() so that it goes back to the
unusual behavior. The patch would look like this:

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 848b56efc9d7..f0c03e432648 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3314,6 +3314,13 @@ bool kmsg_dump_get_line_nolock(struct kmsg_dumper *dumper, bool syslog,
size_t l = 0;
bool ret = false;

+ /*
+ * Callers expect this function to never fill more than
+ * @size-1 bytes so reduce the provided size.
+ */
+ if (size > 0)
+ size--;
+
prb_rec_init_rd(&r, &info, line, size);

if (!dumper->active)
Since kmsg_dump_get_line() is an exported symbol, my preferred approach
(which changes the semantics) may not be acceptible. In that case, we
can take the 2nd approach.

Please let me know your thoughts so I can post a patch for 5.10-stable
and 5.11-rc.

John Ogness

\
 
 \ /
  Last update: 2021-01-13 21:03    [W:0.034 / U:2.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site