seq_buf_putc() and strings

seq_buf_putc() and strings

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

Hi Steven,


I've been looking at rewriting seq_buf with pointers instead of sizes.
While doing that, I've found one place which writes to seq_buf without
forming a proper string (that is, a null-terminated string).

	$ git show v7.3:lib/seq_buf.c | grepc seq_buf_putc
	(standard input):int seq_buf_putc(struct seq_buf *s, unsigned char c)
	{
		WARN_ON(s->size == 0);

		if (seq_buf_can_fit(s, 1)) {
			s->buffer[s->len++] = c;
			return 0;
		}
		seq_buf_set_overflow(s);
		return -1;
	}

I think a safe API for handling strings would have to make sure that it
*always* holds a string.  I think this function should write a NUL byte
after the byte written, and so, I suggest doing this:

	 		s->buffer[s->len++] = c;
	+		s->buffer[s->len] = '\0';

Are there any reasons to not write that NUL byte?


Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: seq_buf_putc() and strings

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1536 bytes --]

> Date: 2026-10-28 18:13:43+0100
> From: Alejandro Colomar <alx+linux-hardening@kernel.org>
>
> Hi Steven,
> 
> 
> I've been looking at rewriting seq_buf with pointers instead of sizes.
> While doing that, I've found one place which writes to seq_buf without
> forming a proper string (that is, a null-terminated string).
> 
> 	$ git show v7.3:lib/seq_buf.c | grepc seq_buf_putc
> 	(standard input):int seq_buf_putc(struct seq_buf *s, unsigned char c)
> 	{
> 		WARN_ON(s->size == 0);
> 
> 		if (seq_buf_can_fit(s, 1)) {
> 			s->buffer[s->len++] = c;
> 			return 0;
> 		}
> 		seq_buf_set_overflow(s);
> 		return -1;
> 	}
> 
> I think a safe API for handling strings would have to make sure that it
> *always* holds a string.  I think this function should write a NUL byte
> after the byte written, and so, I suggest doing this:
> 
> 	 		s->buffer[s->len++] = c;
> 	+		s->buffer[s->len] = '\0';
> 
> Are there any reasons to not write that NUL byte?

Or to simplify, we could rewrite the function entirely as:

	 int seq_buf_putc(struct seq_buf *s, unsigned char c)
	 {
	-       WARN_ON(s->size == 0);
	-
	-       if (seq_buf_can_fit(s, 1)) {
	-               s->buffer[s->len++] = c;
	-               return 0;
	-       }
	-       seq_buf_set_overflow(s);
	-       return -1;
	+       return seq_buf_puts(s, (char [2]){c, '\0'});
	 }

It involves one more function call, but I suspect that overhead is
going to be negligible.


Cheers,
Alex

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: seq_buf_putc() and strings

[Steven Rostedt]

On Wed, 28 Oct 2026 18:13:40 +0100
Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:

> Are there any reasons to not write that NUL byte?

Yes, for speed. seq_buf is used in hotpaths in tracing. I purposely made
the design decision that the seq_buf string is never guaranteed to be NUL
terminated.

-- Steve

Re: seq_buf_putc() and strings

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

Hi Steven,

> Date: 2026-10-28 13:47:55-0400
> From: Steven Rostedt <rostedt@goodmis.org>
>
> On Wed, 28 Oct 2026 18:13:40 +0100
> Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:
> 
> > Are there any reasons to not write that NUL byte?
> 
> Yes, for speed. seq_buf is used in hotpaths in tracing. I purposely made
> the design decision that the seq_buf string is never guaranteed to be NUL
> terminated.

Hmmm, okay; I'll keep that in mind when rewriting.  Thanks!


Cheers,
Alex

> 
> -- Steve

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

seq_buf_used() return value on overflow

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]

Hi Steven,


Another question that arised while rewriting seq_buf.

There's seq_buf_used(), which replaced (in 2014) the previous
SEQ_BUF_USED().  That commit changed the value from
	min((s)->len, (s)->size - 1)
to
	min(s->len, s->size)
but didn't change any other code.  I don't see any mentions about that
off-by-one change in the commit message.  (There are two 'Link:' tags,
but I didn't see any discussion about this patch in those; they seem to
focus on other patches of the same set.)

I wonder, because when changing to use pointers, the good behavior would
be with the -1, so I wonder whether we can go back to the old behavior
easily, or if anything depends on that value.  Was this a regression?
Or was it a silent fix?  Or is it unimportant at the moment?

This is the commit that I'm talking about:

eeab98154dc0 (2014-11-19; "seq_buf: Create seq_buf_used() to find out how much was written")

Here's what I'd like to change:

	diff --git i/include/linux/seq_buf.h w/include/linux/seq_buf.h
	index 05e70ac18ddb..63d077dd8a74 100644
	--- i/include/linux/seq_buf.h
	+++ w/include/linux/seq_buf.h
	@@ -78,7 +78,7 @@
	 /* How much buffer was written? */
	 static inline unsigned int seq_buf_used(struct seq_buf *s)
	 {
	-       return min(s->len, s->size);
	+       return min(s->len, s->size - 1);
	 }
	 
	 /**

The rationale for having that -1 is the following:
Consider a representation of a buffer where s->p holds either a pointer
to the terminating null byte (or the position where it should be written,
if not yet written), or NULL if the buffer has overflown, and s->end
holds unconditionally ARRAY_END() of the buffer.

By doing s->size-1 on overflow, we'd translate it to s->end-1 with the
pointer representation, which happens to point to the null byte (or
where it should be written), which would make this offset quite
consistent.

If we do s->size without the -1, then that offset puts us one-past-the-
last element, which isn't usable in the same places.

If you would accept a patch to add back that -1, let me know and I'll
send it, or feel free to do it yourself adding the following:

	Reported-by: Alejandro Colomar <alx@kernel.org>
	Suggested-by: Alejandro Colomar <alx@kernel.org>

I guess it would be better that I send a patch with a proper commit
message.


Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: seq_buf_used() return value on overflow

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2984 bytes --]

> Date: 2026-10-28 21:59:44+0100
> From: Alejandro Colomar <alx+linux-hardening@kernel.org>
>
> Hi Steven,
> 
> 
> Another question that arised while rewriting seq_buf.
> 
> There's seq_buf_used(), which replaced (in 2014) the previous
> SEQ_BUF_USED().  That commit changed the value from
> 	min((s)->len, (s)->size - 1)
> to
> 	min(s->len, s->size)
> but didn't change any other code.  I don't see any mentions about that
> off-by-one change in the commit message.  (There are two 'Link:' tags,
> but I didn't see any discussion about this patch in those; they seem to
> focus on other patches of the same set.)
> 
> I wonder, because when changing to use pointers, the good behavior would
> be with the -1, so I wonder whether we can go back to the old behavior
> easily, or if anything depends on that value.  Was this a regression?
> Or was it a silent fix?  Or is it unimportant at the moment?
> 
> This is the commit that I'm talking about:
> 
> eeab98154dc0 (2014-11-19; "seq_buf: Create seq_buf_used() to find out how much was written")
> 
> Here's what I'd like to change:
> 
> 	diff --git i/include/linux/seq_buf.h w/include/linux/seq_buf.h
> 	index 05e70ac18ddb..63d077dd8a74 100644
> 	--- i/include/linux/seq_buf.h
> 	+++ w/include/linux/seq_buf.h
> 	@@ -78,7 +78,7 @@
> 	 /* How much buffer was written? */
> 	 static inline unsigned int seq_buf_used(struct seq_buf *s)
> 	 {
> 	-       return min(s->len, s->size);
> 	+       return min(s->len, s->size - 1);
> 	 }
> 	 
> 	 /**
> 
> The rationale for having that -1 is the following:
> Consider a representation of a buffer where s->p holds either a pointer
> to the terminating null byte (or the position where it should be written,
> if not yet written), or NULL if the buffer has overflown, and s->end
> holds unconditionally ARRAY_END() of the buffer.
> 
> By doing s->size-1 on overflow, we'd translate it to s->end-1 with the
> pointer representation, which happens to point to the null byte (or
> where it should be written), which would make this offset quite
> consistent.
> 
> If we do s->size without the -1, then that offset puts us one-past-the-
> last element, which isn't usable in the same places.

Oh, I think I see the reason.  If ->size==0, then -1 would wrap around
and give a huge value.

So, we can't do this for now.  I'll have to do the change at the same
time that I do the other changes (I was hoping I could do some parts
gradually).


Cheers,
Alex

> 
> If you would accept a patch to add back that -1, let me know and I'll
> send it, or feel free to do it yourself adding the following:
> 
> 	Reported-by: Alejandro Colomar <alx@kernel.org>
> 	Suggested-by: Alejandro Colomar <alx@kernel.org>
> 
> I guess it would be better that I send a patch with a proper commit
> message.
> 
> 
> Have a lovely night!
> Alex
> 
> -- 
> <https://www.alejandro-colomar.es>



-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

seq_buf_str() silent truncation (data corruption)

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 1686 bytes --]

Hi Steve,


I believe I've found a bug in seq_buf_str().

Consider the following code:

	char            buf[3];
	struct seq_buf  s;

	seq_buf_init(&s, buf, 3);
	seq_buf_putmem(&s, "foo", 3);  // This will return 0, I believe.

	// At this point, ->len==3 and ->size==3, I believe.

	seq_buf_str(&s);  // BUG

Let's see the implementation of seq_buf_str():

	$ git show v7.3:include/linux/seq_buf.h | grepc seq_buf_str
	(standard input):static inline const char *seq_buf_str(struct seq_buf *s)
	{
		if (WARN_ON(s->size == 0))
			return "";

		if (seq_buf_buffer_left(s))
			s->buffer[s->len] = 0;
		else
			s->buffer[s->size - 1] = 0;

		return s->buffer;
	}

The first check is fine (we don't trigger the WARN_ON()).
seq_buf_buffer_left(s) will return 0 (it returns 's->size - s->len').
This means we'll execute the second branch.  That branch will result in
overwriting the third byte with a '\0', without reporting the overflow.

I believe the following should fix the bug:

	@@ -102,10 +104,12 @@ static inline const char *seq_buf_str(struct seq_buf *s)
		if (WARN_ON(s->size == 0))
			return "";
	 
	-       if (seq_buf_buffer_left(s))
	+       if (seq_buf_buffer_left(s)) {
			s->buffer[s->len] = 0;
	-       else
	+       } else {
	+               seq_buf_set_overflow(s);
			s->buffer[s->size - 1] = 0;
	+       }
	 
		return s->buffer;
	 }

I have not yet researched enough to sign a patch, but please let me know
what you think.  I can prepare a formal patch next week.

NOT-assisted-by: LLM
Reported-by: Alejandro Colomar <alx@kernel.org>


Have a lovely night!
Alex

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: seq_buf_str() silent truncation (data corruption)

[Steven Rostedt]

On Wed, 28 Oct 2026 22:30:38 +0100
Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:

> Let's see the implementation of seq_buf_str():
> 
> 	$ git show v7.3:include/linux/seq_buf.h | grepc seq_buf_str
> 	(standard input):static inline const char *seq_buf_str(struct seq_buf *s)
> 	{
> 		if (WARN_ON(s->size == 0))
> 			return "";
> 
> 		if (seq_buf_buffer_left(s))
> 			s->buffer[s->len] = 0;
> 		else
> 			s->buffer[s->size - 1] = 0;
> 
> 		return s->buffer;
> 	}
> 
> The first check is fine (we don't trigger the WARN_ON()).
> seq_buf_buffer_left(s) will return 0 (it returns 's->size - s->len').
> This means we'll execute the second branch.  That branch will result in
> overwriting the third byte with a '\0', without reporting the overflow.

Yes, Kees and I already discussed this:

  https://lore.kernel.org/all/20261016145928.16eddc9a@gandalf.local.home/

-- Steve