Messages in this thread Patches in this message | | | From | John Ogness <> | Subject | RFC: printk: kmsg_dump_get_line_nolock() buffer overflow | Date | Wed, 13 Jan 2021 21:06:28 +0106 |
| |
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
| |