linux-kernel - [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 17 Oct 2018 23:10:00 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     rostedt@...dmis.org
Cc:     linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer

Currently seq_buf_puts() will happily create a non NULL terminated
string for you in the buffer. This is particularly dangerous if the
buffer is on the stack.

For example:

  char buf[8];
  char secret = "secret";
  struct seq_buf s;

  seq_buf_init(&s, buf, sizeof(buf));
  seq_buf_puts(&s, "foo");
  printk("Message is %s\n", buf);

Can result in:

  Message is fooªªªªªsecret

We could require all users to memset() their buffer to NULL before
use. But that seems likely to be forgotten and lead to bugs.

Instead we can change seq_buf_puts() to always leave the buffer in a
NULL terminated state.

The only downside is that this makes the buffer 1 character smaller
for seq_buf_puts(), but that seems like a good trade off.

Signed-off-by: Michael Ellerman <mpe@...erman.id.au>
---
 lib/seq_buf.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

I recently merged a patch which actually hit this behaviour. I worked
around it by using seq_buf_printf(), but it would be good to fix the
problem at the source.

diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 11f2ae0f9099..b1570204cde3 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -144,9 +144,13 @@ int seq_buf_puts(struct seq_buf *s, const char *str)

 	WARN_ON(s->size == 0);

+	/* Add 1 to len for the trailing NULL which must be there */
+	len += 1;
+
 	if (seq_buf_can_fit(s, len)) {
 		memcpy(s->buffer + s->len, str, len);
-		s->len += len;
+		/* Don't count the trailing NULL against the capacity */
+		s->len += len - 1;
 		return 0;
 	}
 	seq_buf_set_overflow(s);
-- 
2.17.1