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

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

[Kees Cook]

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

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

[Alejandro Colomar]

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

Hi Kees, Steve,

> Date: 2026-10-28 22:32:54-0700
> From: 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.

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.


Have a lovely day!
Alex

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

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

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

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

[Steven Rostedt]

On Thu, 29 Oct 2026 12:47:04 +0100
Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:

> 
> 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().

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

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


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

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

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

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

-- Steve


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

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

[Alejandro Colomar]

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

Hi Steve,

> Date: 2026-10-29 09:18:45-0400
> From: Steven Rostedt <rostedt@goodmis.org>
>
> On Thu, 29 Oct 2026 12:47:04 +0100
> Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:
> 
> > 
> > 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().
> 
> > 
> > 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.
> 
> 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.

> > > @@ -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?
> 
> > > -	if (seq_buf_buffer_left(s))
> > > +	if (seq_buf_buffer_left(s)) {
> 
> 		if (s->len && !s->buffer[s->len - 1])
> 			return;
> 
> > >  		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.

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()).


Cheers,
Alex

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

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

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

[Andy Shevchenko]

On Thu, Oct 29, 2026 at 12:47:04PM +0100, Alejandro Colomar wrote:

...

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

/dev/null is perfectly valid device, although I dunno if seq_but designed
in a way to support this case.

-- 
With Best Regards,
Andy Shevchenko

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

[Steven Rostedt]

On Thu, 29 Oct 2026 14:52:47 +0100
Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:

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

Right.

> 
> > > > @@ -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?
> >   
> > > > -	if (seq_buf_buffer_left(s))
> > > > +	if (seq_buf_buffer_left(s)) {  
> > 
> > 		if (s->len && !s->buffer[s->len - 1])
> > 			return;
> >   
> > > >  		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.  
> 
> 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.


> 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()).

"end" is just fine.

-- Steve