RFC: printk: kmsg_dump_get_line_nolock() buffer overflow

RFC: printk: kmsg_dump_get_line_nolock() buffer overflow

[John Ogness]

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

Re: RFC: printk: kmsg_dump_get_line_nolock() buffer overflow

[Petr Mladek]

On Wed 2021-01-13 21:06:28, John Ogness wrote:
> 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.

Just to make it clear. There is no visible change in
kmsg_dump_get_line_nolock() in the commit 896fbe20b4e2
("printk: use the lockless ringbuffer").

The eal change happened in record_printk_text():

-		if (buf) {
-			if (prefix_len + text_len + 1 >= size - len)
+		/*
+		 * Truncate the text if there is not enough space to add the
+		 * prefix and a trailing newline.
+		 */
+		if (len + prefix_len + text_len + 1 > buf_size) {
+			/* Drop even the current line if no space. */
+			if (len + prefix_len + line_len + 1 > buf_size)
 				break;

It replaced ">=" with ">".

It is pitty that I have missed this. I remember that I discussed
exactly this problem before, see
https://lore.kernel.org/lkml/20190710080402.ab3f4qfnvez6dhtc@axis.com/

And I did exactly the same mistake. I have missed the two users in
"arch/powerpc" and "arch/um".

> 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';

It is clear that this problem happens repeatedly.

Now, the change in record_printk_text() behavior affects also other
callers. For example, syslog_print() fills the buffer completely
as well now. I could imagine a userspace code that does the same
mistake and it works just by chance.

We should restore the original record_printk_text() behavior
and add the comment explaining why it is done this way. And
I would even explicitly add the trailing '\0' as suggested at
https://lore.kernel.org/lkml/20190710121049.rwhk7fknfzn3cfkz@pathway.suse.cz/#t

Best Regards,
Petr

Re: RFC: printk: kmsg_dump_get_line_nolock() buffer overflow

[John Ogness]

On 2021-01-14, Petr Mladek <pmladek@suse.com> wrote:
> It is pitty that I have missed this. I remember that I discussed
> exactly this problem before, see
> https://lore.kernel.org/lkml/20190710080402.ab3f4qfnvez6dhtc@axis.com/
>
> And I did exactly the same mistake. I have missed the two users in
> "arch/powerpc" and "arch/um".
>
> It is clear that this problem happens repeatedly.

Yes, because the semantics are poor and undocumented.

> Now, the change in record_printk_text() behavior affects also other
> callers. For example, syslog_print() fills the buffer completely
> as well now. I could imagine a userspace code that does the same
> mistake and it works just by chance.

No, syslog_print() works fine. There are only 2 users that think they
can blindly add a byte at buffer[len]. Their code looks scary just
seeing it.

> We should restore the original record_printk_text() behavior
> and add the comment explaining why it is done this way.

OK.

> And I would even explicitly add the trailing '\0' as suggested at
> https://lore.kernel.org/lkml/20190710121049.rwhk7fknfzn3cfkz@pathway.suse.cz/#t

OK. But then this becomes official semantics so powerpc/um no longer
need to append a terminator.

John Ogness

Re: RFC: printk: kmsg_dump_get_line_nolock() buffer overflow

[Petr Mladek]

On Thu 2021-01-14 17:06:59, John Ogness wrote:
> On 2021-01-14, Petr Mladek <pmladek@suse.com> wrote:
> > It is pitty that I have missed this. I remember that I discussed
> > exactly this problem before, see
> > https://lore.kernel.org/lkml/20190710080402.ab3f4qfnvez6dhtc@axis.com/
> >
> > And I did exactly the same mistake. I have missed the two users in
> > "arch/powerpc" and "arch/um".
> >
> > It is clear that this problem happens repeatedly.
> 
> Yes, because the semantics are poor and undocumented.

I fully agree.


> > Now, the change in record_printk_text() behavior affects also other
> > callers. For example, syslog_print() fills the buffer completely
> > as well now. I could imagine a userspace code that does the same
> > mistake and it works just by chance.
> 
> No, syslog_print() works fine.

Not really. It fills the entire buffer provided by the user space now.
It never filled the last byte before. User space might do exactly
the same mistake:

	len = syslog(SYSLOG_ACTION_READ, buf, sizeof(*buf));
	buf[len] = '\0';

This worked before and it causes overflow now.

> There are only 2 users that think they
> can blindly add a byte at buffer[len]. Their code looks scary just
> seeing it.

Except that the functions are exported. I know that breaking external
modules that do ugly things might motivate them to upstream them but...


> > We should restore the original record_printk_text() behavior
> > and add the comment explaining why it is done this way.
> 
> OK.
> 
> > And I would even explicitly add the trailing '\0' as suggested at
> > https://lore.kernel.org/lkml/20190710121049.rwhk7fknfzn3cfkz@pathway.suse.cz/#t
> 
> OK. But then this becomes official semantics so powerpc/um no longer
> need to append a terminator.

We either risk the change of the semantic and break external code.

Or we should make it official. The '\0' will not be used by most of
the API users. But it will make it more safe. The free byte will be
there anyway.

Best Regards,
Petr