[PATCH] seq_buf: Mark a seq_buf overflowed when terminating it loses a byte

[Kees Cook <kees@kernel.org>]

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 <rostedt@goodmis.org>
Closes: https://lore.kernel.org/all/20261016145928.16eddc9a@gandalf.local.home/
Fixes: 8d1478bfd63be ("seq_buf: Add seq_buf_strlen()")
Signed-off-by: Kees Cook <kees@kernel.org>
---
 include/linux/seq_buf.h   | 19 ++++++++++-----
 lib/tests/seq_buf_kunit.c | 49 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
index f006f2654e4c..c38b5eccc212 100644
--- a/include/linux/seq_buf.h
+++ b/include/linux/seq_buf.h
@@ -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.
diff --git a/lib/tests/seq_buf_kunit.c b/lib/tests/seq_buf_kunit.c
index fc53e9ef844c..a67ed6d7b517 100644
--- a/lib/tests/seq_buf_kunit.c
+++ b/lib/tests/seq_buf_kunit.c
@@ -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),
 	{}
 };
-- 
2.55.0