[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20181017104826.7f6fb605@gandalf.local.home>
Date: Wed, 17 Oct 2018 10:48:26 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Michael Ellerman <mpe@...erman.id.au>
Cc: linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer
On Wed, 17 Oct 2018 23:10:00 +1100
Michael Ellerman <mpe@...erman.id.au> wrote:
> 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>
>
It's been on my todo list for some time, and that was to add this
(trace-cmd had this for years). Would this be useful?
Instead of always writing a nul pointer, just terminate it when you are
done.
-- Steve
diff --git a/lib/seq_buf.c b/lib/seq_buf.c
index 11f2ae0f9099..46f2a8b3e733 100644
--- a/lib/seq_buf.c
+++ b/lib/seq_buf.c
@@ -175,6 +175,29 @@ int seq_buf_putc(struct seq_buf *s, unsigned char c)
}
/**
+ * seq_buf_terminate - force the buffer to end in a nul character
+ * @s: seq_buf descriptor
+ *
+ * Make sure the current buffer terminates with a nul '\0' character.
+ * This does not increment the length of the buffer.
+ *
+ * Returns 0 if there was room to add the nul character.
+ * -1 if there was not room. But a nul character was
+ * still added, overwriting the last character in the buffer.
+ */
+int seq_buf_terminate(struct seq_buf *s)
+{
+ WARN_ON(s->size == 0);
+
+ if (seq_buf_can_fit(s, 1)) {
+ s->buffer[s->len] = '\0';
+ return 0;
+ }
+ s->buffer[--s->len] = '\0';
+ return -1;
+}
+
+/**
* seq_buf_putmem - write raw data into the sequenc buffer
* @s: seq_buf descriptor
* @mem: The raw memory to copy into the buffer
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
