seq_buf: Mark a seq_buf overflowed when terminating it loses a byte

Date: 2026-10-28 22:32:54-0700
From: Kees Cook <[email protected]>

When there is no room after the data for a NUL, __seq_buf_terminate() writes it over the last byte of the buffer. An overflowed seq_buf has already lost data, but one that is exactly full, as seq_buf_putc(), seq_buf_putmem(), and seq_buf_commit() can leave it, has not: the NUL replaces the last byte written, and the seq_buf still does not report an overflow. seq_buf_str(), seq_buf_strlen(), and seq_buf_terminate() all go through it, so each of them can silently truncate what was written, and seq_buf_pop() then returns the NUL in place of the byte it replaced.

Mark the seq_buf overflowed in that case with seq_buf_set_overflow(), as suggested in review. Nothing is left between "len" and "size" for it to clear there, and seq_buf_used() still counts the lost byte.

No current caller checks for overflow after terminating a seq_buf that can be left exactly full, so none of them behaves differently.

Add a test that fills a seq_buf exactly and checks that each of the three accessors marks it overflowed, that seq_buf_pop() then returns -1, and that a seq_buf with room for the NUL is not marked. Update the existing tests that terminate an exactly full seq_buf to expect it, and move the length check in the seq_buf_do_printk() test after the run, so that the buffer still reaches seq_buf_do_printk() exactly full.

Tests passed under qemu on ARCH=x86_64 with GCC 16.2.0 and CONFIG_KASAN=y, and on big-endian ARCH=s390 with GCC s390x-linux-gnu 16.2.0, and the new test fails with the fix reverted.

Reported-by Steven Rostedt <[email protected]>

Closes https://lore.kernel.org/all/[email protected]/ open here

Fixes 8d1478bfd63be ("seq_buf: Add seq_buf_strlen()")

Signed-off-by Kees Cook <[email protected]>

---
 include/linux/seq_buf.h   | 19 ++++++++++-----
 lib/tests/seq_buf_kunit.c | 49 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 8 deletions(-)

include/linux/seq_buf.h +13 -6

index f006f2654e4c..c38b5eccc212 100644
@@ -128,8 +128,8 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
 
 /*
  * NUL-terminate the buffer in @s: directly after the data when there is
- * room for it, otherwise in the last byte of the buffer. @s->size must not
- * be zero.
+ * room for it, otherwise in the last byte of the buffer, which marks @s as
+ * overflowed. @s->size must not be zero.
  *
  * Returns: the offset of the NUL.
  */

@@ -137,10 +137,16 @@ static inline size_t __seq_buf_terminate(struct seq_buf *s)
 {
 	size_t end;
 
-	if (seq_buf_buffer_left(s))
+	if (seq_buf_buffer_left(s)) {
 		end = s->len;
-	else
+	} else {
 		end = s->size - 1;
+		/*
+		 * About to overwrite a written byte in the last position
+		 * in the buffer, so it must be considered overflowed now.
+		 */
+		seq_buf_set_overflow(s);
+	}
 
 	s->buffer[end] = 0;
 

@@ -158,7 +164,8 @@ static inline size_t __seq_buf_terminate(struct seq_buf *s)
  * the last byte of the buffer is zeroed, and the len will still
  * point passed it. The same happens when the buffer is exactly
  * full: the NUL takes the place of the last byte written, which is
- * lost, though seq_buf_used() still counts it.
+ * lost, so @s is marked as overflowed, though seq_buf_used() still
+ * counts that byte.
  *
  * A zero-sized seq_buf has nowhere to put a NUL, so the empty string
  * is returned instead of writing to @s->buffer. Any other seq_buf

@@ -189,7 +196,7 @@ static inline const char *seq_buf_str(struct seq_buf *s)
  * without walking it. Unlike seq_buf_used(), this does not count the byte
  * given up to the NUL when the buffer is full or has overflowed. When the
  * buffer is exactly full, that byte is the last one written, and calling
- * either function loses it.
+ * either function loses it and marks @s as overflowed.
  *
  * A zero-sized seq_buf holds no string, so 0 is returned without writing
  * to @s->buffer, matching what seq_buf_str() returns for one.

lib/tests/seq_buf_kunit.c +47 -2

index fc53e9ef844c..a67ed6d7b517 100644
@@ -504,13 +504,13 @@ static void seq_buf_do_printk_test(struct kunit *test)
 	seq_buf_putc(&s, '!');
 	KUNIT_ASSERT_FALSE(test, seq_buf_has_overflowed(&s));
 	KUNIT_ASSERT_EQ(test, seq_buf_used(&s), 8);
-	KUNIT_ASSERT_EQ(test, strlen(seq_buf_str(&s)), 7);
 
 	seq_buf_printk_run(&capture, &s);
 
 	/* The one line that was written, and nothing after it. */
 	KUNIT_EXPECT_EQ(test, seq_buf_printk_marked, 1);
 	KUNIT_EXPECT_EQ(test, seq_buf_printk_empty, 0);
+	KUNIT_EXPECT_EQ(test, strlen(seq_buf_str(&s)), 7);
 
 	/* Check that lines without a trailing newline are shown. */
 	seq_buf_puts(&t, SEQ_BUF_PRINTK_MARKER "\n" SEQ_BUF_PRINTK_MARKER);

@@ -630,7 +630,8 @@ static void seq_buf_strlen_full_test(struct kunit *test)
 	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
 	KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 4);
 	KUNIT_EXPECT_EQ(test, seq_buf_strlen(&s), 3);
-	/* seq_buf_strlen() terminates the buffer by itself. */
+	/* seq_buf_strlen() terminates the buffer by itself, losing the 'd'. */
+	KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
 	KUNIT_EXPECT_EQ(test, s.buffer[3], '\0');
 	KUNIT_EXPECT_EQ(test, strnlen(s.buffer, s.size), 3);
 	KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "abc");

@@ -649,6 +650,7 @@ static void seq_buf_strlen_full_test(struct kunit *test)
 	seq_buf_commit(&t, len);
 	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&t));
 	KUNIT_EXPECT_EQ(test, seq_buf_strlen(&t), 7);
+	KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&t));
 	KUNIT_EXPECT_EQ(test, t.buffer[7], '\0');
 	KUNIT_EXPECT_EQ(test, strnlen(t.buffer, t.size), 7);
 	KUNIT_EXPECT_STREQ(test, seq_buf_str(&t), "zzzzzzz");

@@ -735,6 +737,48 @@ static void seq_buf_terminate_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, buf, "untouched");
 }
 
+static void seq_buf_terminate_full_test(struct kunit *test)
+{
+	DECLARE_SEQ_BUF(s, 4);
+	DECLARE_SEQ_BUF(t, 4);
+	DECLARE_SEQ_BUF(u, 4);
+	DECLARE_SEQ_BUF(v, 4);
+
+	/*
+	 * Terminating a buffer that is exactly full puts the NUL over the
+	 * last byte written, so what it holds is truncated, and each of
+	 * seq_buf_str(), seq_buf_strlen() and seq_buf_terminate() must mark
+	 * it overflowed.
+	 */
+	seq_buf_putmem(&s, "abcd", 4);
+	KUNIT_ASSERT_FALSE(test, seq_buf_has_overflowed(&s));
+	KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "abc");
+	KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
+	KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 4);
+	/* The NUL must not be popped in place of the 'd' it replaced. */
+	KUNIT_EXPECT_EQ(test, seq_buf_pop(&s), -1);
+	KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "abc");
+
+	seq_buf_putmem(&t, "abcd", 4);
+	KUNIT_ASSERT_FALSE(test, seq_buf_has_overflowed(&t));
+	KUNIT_EXPECT_EQ(test, seq_buf_strlen(&t), 3);
+	KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&t));
+
+	seq_buf_putmem(&u, "abcd", 4);
+	KUNIT_ASSERT_FALSE(test, seq_buf_has_overflowed(&u));
+	seq_buf_terminate(&u);
+	KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&u));
+	KUNIT_EXPECT_STREQ(test, u.buffer, "abc");
+
+	/* With room left for the NUL, nothing is lost. */
+	seq_buf_putmem(&v, "abc", 3);
+	KUNIT_EXPECT_STREQ(test, seq_buf_str(&v), "abc");
+	KUNIT_EXPECT_EQ(test, seq_buf_strlen(&v), 3);
+	seq_buf_terminate(&v);
+	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&v));
+	KUNIT_EXPECT_EQ(test, seq_buf_pop(&v), 'c');
+}
+
 static struct kunit_case seq_buf_test_cases[] = {
 	KUNIT_CASE(seq_buf_init_test),
 	KUNIT_CASE(seq_buf_init_append_test),

@@ -760,6 +804,7 @@ static struct kunit_case seq_buf_test_cases[] = {
 	KUNIT_CASE(seq_buf_strlen_embedded_nul_test),
 	KUNIT_CASE(seq_buf_strlen_zero_size_test),
 	KUNIT_CASE(seq_buf_terminate_test),
+	KUNIT_CASE(seq_buf_terminate_full_test),
 	KUNIT_CASE(seq_buf_do_printk_test),
 	{}
 };

Signature · 1 line

2.55.0

This fix in theory solves the issue. However, I wonder if this is maybe too late to report the overflow. Callers normally expect the overflow to happen while they're performing their writes, and maybe they don't expect the overflow to happen at this point, so they might forget to test, and keep going with truncated data.

How about not allowing a full buffer without a NUL terminator in the first place? That is, we could report a _putmem or _putc operation that writes a non-zero byte to the last element in the buffer as being overflowed.

This would waste one byte, but would prevent users from having to expect a possible overflow when reading a buffer.

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 139

Quoted 4 lines

include/linux/seq_buf.h +0 -0

@@ -137,10 +137,16 @@ static inline size_t __seq_buf_terminate(struct seq_buf *s)
 {
 	size_t end;
 

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 140

Quoted 2 lines

include/linux/seq_buf.h +1 -1

-	if (seq_buf_buffer_left(s))
+	if (seq_buf_buffer_left(s)) {

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 143

Quoted 4 lines

include/linux/seq_buf.h +1 -1

 		end = s->len;
-	else
+	} else {
 		end = s->size - 1;

Quoted 9 lines

include/linux/seq_buf.h +6 -0

+		/*
+		 * About to overwrite a written byte in the last position
+		 * in the buffer, so it must be considered overflowed now.
+		 */
+		seq_buf_set_overflow(s);
+	}
 
 	s->buffer[end] = 0;
 

Date: 2026-10-29 09:18:45-0400
From: Steven Rostedt <[email protected]>

Quoted 6 lines · On Thu, 29 Oct 2026 12:47:04 +0100 Alejandro Colomar…

This fix in theory solves the issue. However, I wonder if this is maybe too late to report the overflow. Callers normally expect the overflow to happen while they're performing their writes, and maybe they don't expect the overflow to happen at this point, so they might forget to test, and keep going with truncated data.

No, the overflow should be checked when they are done with the seq_buf. Which means after they do either seq_buf_str() or seq_buf_terimnate().

Quoted 6 lines

How about not allowing a full buffer without a NUL terminator in the first place? That is, we could report a _putmem or _putc operation that writes a non-zero byte to the last element in the buffer as being overflowed.

Quoted 2 lines

This would waste one byte, but would prevent users from having to expect a possible overflow when reading a buffer.

This only happens when the user expects the string to have a nul terminating byte. Don't think too much into this.

Quoted 4 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 139

Quoted 4 lines

include/linux/seq_buf.h +0 -0

@@ -137,10 +137,16 @@ static inline size_t __seq_buf_terminate(struct seq_buf *s)
 {
 	size_t end;
 

Hmm, I was thinking that this should first check if the string is already terminated with a nul byte. Why add another one if it is?

Quoted 2 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 140

Quoted 2 lines

include/linux/seq_buf.h +1 -1

-	if (seq_buf_buffer_left(s))
+	if (seq_buf_buffer_left(s)) {

		if (s->len && !s->buffer[s->len - 1])
			return;

Quoted 4 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 143

Quoted 4 lines

include/linux/seq_buf.h +1 -1

 		end = s->len;
-	else
+	} else {
 		end = s->size - 1;

		if (!s->buffer[end])
			return;

Otherwise this overflows the buffer even when it is already terminated.

Quoted 3 lines

This only happens when the user expects the string to have a nul terminating byte. Don't think too much into this.

Hmmmm, sounds reasonable under the point of view that users shouldn't expect these buffers to contain strings. Then, seq_buf_str() would obviously be a write operation, since it transforms to a string from something that wasn't a string.

Quoted 23 lines

Quoted 5 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 139

Quoted 5 lines

include/linux/seq_buf.h +0 -0

@@ -137,10 +137,16 @@ static inline size_t __seq_buf_terminate(struct seq_buf *s)
 {
 	size_t end;
 

Hmm, I was thinking that this should first check if the string is already terminated with a nul byte. Why add another one if it is?

Quoted 2 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 140

Quoted 2 lines

include/linux/seq_buf.h +1 -1

-	if (seq_buf_buffer_left(s))
+	if (seq_buf_buffer_left(s)) {  

		if (s->len && !s->buffer[s->len - 1])
			return;

Quoted 4 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 143

Quoted 4 lines

include/linux/seq_buf.h +1 -1

 		end = s->len;
-	else
+	} else {
 		end = s->size - 1;  

		if (!s->buffer[end])
			return;

Quoted 1 line

Otherwise this overflows the buffer even when it is already terminated.

If it is already terminated, s->buffer+s->len points to the NUL, right?

Is so, then we'd only reach this second branch when the buffer has already overflowed (seq_buf_has_overflowed(s)==true). If the buffer has not overflowed, s->len<s->size, and thus we'd fall in the first branch.

If we have already overflowed, we shouldn't be concerned about further overflowing, right? And on overflow, if there's a NUL, it certainly must be at s->buffer+s->size-1, so overwriting it with another NUL wouldn't do any harm. If this helps us avoid a check (and thus a branch), it would be worth it, right?

BTW, I'd very much suggest renaming that 'end' local variable to 'nul', since it is the offset of the NUL byte. 'end' could be confused with the end of the buffer (as in ARRAY_END()).

Date: 2026-10-29 10:18:38-0400
From: Steven Rostedt <[email protected]>

Quoted 22 lines

Quoted 22 lines

Quoted 4 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 139

Quoted 4 lines

include/linux/seq_buf.h +0 -0

@@ -137,10 +137,16 @@ static inline size_t __seq_buf_terminate(struct seq_buf *s)
 {
 	size_t end;
 

Hmm, I was thinking that this should first check if the string is already terminated with a nul byte. Why add another one if it is?

Quoted 2 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 140

Quoted 2 lines

include/linux/seq_buf.h +1 -1

-	if (seq_buf_buffer_left(s))
+	if (seq_buf_buffer_left(s)) {  

		if (s->len && !s->buffer[s->len - 1])
			return;

Quoted 4 lines

include/linux/seq_buf.h static inline size_t __seq_buf_terminate(struct seq_buf *s) · line 143

Quoted 4 lines

include/linux/seq_buf.h +1 -1

 		end = s->len;
-	else
+	} else {
 		end = s->size - 1;  

		if (!s->buffer[end])
			return;

Quoted 3 lines

Quoted 1 line

Otherwise this overflows the buffer even when it is already terminated.

If it is already terminated, s->buffer+s->len points to the NUL, right?

Not necessarially, a seq_buf_putmem() could have added the NUL terminator. Then calling seq_buf_str() or seq_buf_terminate() should not overflow it.

Hmm, I was thinking that this should first check if the string is already terminated with a nul byte. Why add another one if it is?

How about not allowing a full buffer without a NUL terminator in the first place?