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

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

[Michael Ellerman]

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@ellerman.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

Re: [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer

[Jann Horn]

On Wed, Oct 17, 2018 at 2:10 PM Michael Ellerman <mpe@ellerman.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.

After this, you can also simplify rdt_last_cmd_status_show(), right?

> ---
>  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 */

Nit: In the comments, I would prefer either "null byte" or "NUL"
instead of "NULL" when talking about something that is not a pointer.

> +       len += 1;

It looks like you're using an "unsigned int" for the length, meaning
that this can in theory (e.g. when operating on a string from a big
vmalloc buffer) overflow. You should be using size_t here.

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

Re: [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer

[Steven Rostedt]

On Wed, 17 Oct 2018 14:26:37 +0200
Jann Horn <jannh@google.com> wrote:

> > 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 */  
> 
> Nit: In the comments, I would prefer either "null byte" or "NUL"
> instead of "NULL" when talking about something that is not a pointer.
> 
> > +       len += 1;  
> 
> It looks like you're using an "unsigned int" for the length, meaning
> that this can in theory (e.g. when operating on a string from a big
> vmalloc buffer) overflow. You should be using size_t here.

seq_buf is not meant for gigabytes of data. We'll change it when that
ever happens.

-- Steve


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

Re: [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer

[Steven Rostedt]

On Wed, 17 Oct 2018 23:10:00 +1100
Michael Ellerman <mpe@ellerman.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@ellerman.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

Re: [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer

[Kees Cook]

On Wed, Oct 17, 2018 at 5:10 AM, Michael Ellerman <mpe@ellerman.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@ellerman.id.au>

Yes, please! :) I prefer keeping the string terminated over needing to
remember to do it later.

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

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



-- 
Kees Cook
Pixel Security

Re: [PATCH] seq_buf: Make seq_buf_puts() NULL terminate the buffer

[Michael Ellerman]

Jann Horn <jannh@google.com> writes:
> On Wed, Oct 17, 2018 at 2:10 PM Michael Ellerman <mpe@ellerman.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.
>
> After this, you can also simplify rdt_last_cmd_status_show(), right?

Yes.

We also have a seq_buf_printf() in powerpc code that is printing a fixed
string purely to get NULL termination, so that can become a
seq_buf_puts().

>> 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 */
>
> Nit: In the comments, I would prefer either "null byte" or "NUL"
> instead of "NULL" when talking about something that is not a pointer.

Hmm yeah I guess. I think of them as being more or less the same thing,
or at least interchangeable, but that's a bit sloppy.

I'll send a v2 with "null byte".

>> +       len += 1;
>
> It looks like you're using an "unsigned int" for the length, meaning
> that this can in theory (e.g. when operating on a string from a big
> vmalloc buffer) overflow. You should be using size_t here.

Yes you're right.

And if len overflows to zero above ..

>>         if (seq_buf_can_fit(s, len)) {

This will return true.

>>                 memcpy(s->buffer + s->len, str, len);
>> -               s->len += len;
>> +               /* Don't count the trailing NULL against the capacity */
>> +               s->len += len - 1;

And then here s->len becomes UINT_MAX.

I think. Which is probably not what we want.

I'll send a patch to switch to size_t in there.

cheers