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

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 30 Oct 2026 12:20:54 -0700
Kees Cook <kees@kernel.org> wrote:

> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index f006f2654e4c..1b38c3844e96 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -128,8 +128,9 @@ 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. Data that already ends in a NUL is left as it is, and @s is
> + * not marked. @s->size must not be zero.

Hmm, "left as is" I think sounds better than "left as it is". But either
way, I think the ", and @s is not marked" can be removed as saying the data
is left as is, already covers that.

>   *
>   * Returns: the offset of the NUL.
>   */
> @@ -137,10 +138,20 @@ 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)) {
> +		if (s->len && !s->buffer[s->len - 1])
> +			return s->len - 1;
>  		end = s->len;
> -	else
> +	} else {
>  		end = s->size - 1;
> +		if (!s->buffer[end])
> +			return end;
> +		/*
> +		 * About to overwrite a written byte in the last position
> +		 * in the buffer, so it must be considered overflowed now.

Have you been reading software requirement specs? The "must" seems a bit
excessive ;-)

How about just:

		* The last byte of data has been overwritten by a NUL
		* character, so mark the seq_buf as overflowed.


> +		 */
> +		seq_buf_set_overflow(s);
> +	}
>  
>  	s->buffer[end] = 0;
>  
> @@ -158,7 +169,9 @@ 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. When that byte is a NUL already, nothing is lost
> + * and @s is left as it is.

Hmm, "as it is" or "as is", whichever. Maybe it's a colloquialism for me.

>   *
>   * 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 +202,9 @@ 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, unless it is a NUL
> + * itself. Data that ends in a NUL is already terminated, so its length
> + * does not count that NUL.
>   *
>   * 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..7f9cd5855922 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,82 @@ 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);
> +
> +	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 void seq_buf_terminate_written_nul_test(struct kunit *test)
> +{
> +	DECLARE_SEQ_BUF(s, 4);
> +	DECLARE_SEQ_BUF(t, 4);
> +	DECLARE_SEQ_BUF(u, 4);
> +	DECLARE_SEQ_BUF(v, 8);
> +
> +	/*
> +	 * A buffer filled exactly by data that ends in its own NUL is
> +	 * already terminated, so nothing written is lost, and none of
> +	 * seq_buf_str(), seq_buf_strlen() and seq_buf_terminate() may mark
> +	 * it overflowed.
> +	 */
> +	seq_buf_putmem(&s, "abc", 4);
> +	KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), "abc");
> +	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&s));
> +	KUNIT_EXPECT_EQ(test, seq_buf_used(&s), 4);
> +
> +	seq_buf_putmem(&t, "abc", 4);
> +	KUNIT_EXPECT_EQ(test, seq_buf_strlen(&t), 3);
> +	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&t));
> +
> +	seq_buf_putmem(&u, "abc", 4);
> +	seq_buf_terminate(&u);
> +	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&u));
> +	KUNIT_EXPECT_STREQ(test, u.buffer, "abc");
> +
> +	/* With room left, the length still ends at the NUL that was written. */
> +	seq_buf_putmem(&v, "abc", 4);
> +	KUNIT_EXPECT_EQ(test, seq_buf_strlen(&v), 3);
> +	KUNIT_EXPECT_STREQ(test, seq_buf_str(&v), "abc");
> +	KUNIT_EXPECT_FALSE(test, seq_buf_has_overflowed(&v));
> +	KUNIT_EXPECT_EQ(test, seq_buf_used(&v), 4);
> +}
> +
>  static struct kunit_case seq_buf_test_cases[] = {
>  	KUNIT_CASE(seq_buf_init_test),
>  	KUNIT_CASE(seq_buf_init_append_test),
> @@ -760,6 +838,8 @@ 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_terminate_written_nul_test),
>  	KUNIT_CASE(seq_buf_do_printk_test),
>  	{}
>  };

Rest looks good.

-- Steve