Re: [PATCH v5 05/12] seq_buf: Clear what a writer did not claim when a seq_buf overflows

[David Laight <david.laight.linux@gmail.com>]

On Mon,  5 Oct 2026 08:56:55 -0700
Kees Cook <kees@kernel.org> wrote:

> Using seq_buf_set_overflow() would leave the bytes between "len"
> and "size" untouched, so if seq_buf_str() is used on an overflowed
> seq_buf, those bytes may be exposed. For any paths that don't claim
> partially written bytes, by setting "len = size" before calling
> seq_buf_set_overflow(), wipe the unclaimed bytes. The seq_buf_puts()
> and related APIs already claim those bytes now, so only the unclaimed
> cases remain. A specific example of this was seq_buf_path() which uses
> d_path() and would write to the tail before discovering it was out
> of space, and would correctly mark a seq_buf as overflowed, but the
> path fragment would be left over.
> 
> Clear from len to the end of the buffer in seq_buf_set_overflow(), which
> every overflow goes through, including seq_buf_commit() with a negative
> count.
> 
> Add a test that fills a seq_buf, leaves it too little room for a path, and
> checks that nothing of the path is left in the buffer. The tests run before
> anything writable is mounted, so it takes its file from shmem.
> 
> seq_buf_path() was not exported, so the test failed to link as a
> module. Export it.
> 
> 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
> test builds as a module (CONFIG_SEQ_BUF_KUNIT_TEST=m).
> 
> Assisted-by: LLM
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
>  include/linux/seq_buf.h   |  8 +++++--
>  lib/seq_buf.c             |  5 +++++
>  lib/tests/seq_buf_kunit.c | 44 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> index 77e76e283370..0c0a0db04b09 100644
> --- a/include/linux/seq_buf.h
> +++ b/include/linux/seq_buf.h
> @@ -5,6 +5,7 @@
>  #include <linux/bug.h>
>  #include <linux/minmax.h>
>  #include <linux/seq_file.h>
> +#include <linux/string.h>
>  #include <linux/types.h>
>  
>  /*
> @@ -58,12 +59,15 @@ seq_buf_has_overflowed(struct seq_buf *s)
>  /*
>   * Mark @s as overflowed, which discards the length of what it holds. The
>   * bytes up to its last one are the string from then on, as that is where
> - * seq_buf_str() terminates it, so a caller that could not fill the buffer
> - * has to NUL them itself before calling this.
> + * seq_buf_str() terminates it, so clear whatever was not written: a writer
> + * sets len to how much it filled, and anything past that was never adopted.
>   */
>  static inline void
>  seq_buf_set_overflow(struct seq_buf *s)
>  {
> +	if (s->len < s->size)
> +		memset(s->buffer + s->len, 0, s->size - s->len);

What is the performance impact of zeroing the buffer?
I don't think it would be a good idea to be zeroing the buffer on entry
either (I've not looked to see it that happens - but there will be PAGE_SIZE
buffers (maybe 64k) that get a a small number of characters written to them).
Writing a single '\0' really ought to be enough.

David

> +
>  	s->len = s->size + 1;
>  }
>  
> diff --git a/lib/seq_buf.c b/lib/seq_buf.c
> index 60e9eadb3ef7..8da2e9447adf 100644
> --- a/lib/seq_buf.c
> +++ b/lib/seq_buf.c
> @@ -76,6 +76,8 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
>  			s->len += len;
>  			return 0;
>  		}
> +		/* vsnprintf() wrote as much as fits, so none of it is stale */
> +		s->len = s->size;
>  	}
>  	seq_buf_set_overflow(s);
>  	return -1;
> @@ -164,6 +166,8 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
>  			s->len += ret;
>  			return 0;
>  		}
> +		/* bstr_printf() wrote as much as fits, so none of it is stale */
> +		s->len = s->size;
>  	}
>  	seq_buf_set_overflow(s);
>  	return -1;
> @@ -343,6 +347,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
>  
>  	return res;
>  }
> +EXPORT_SYMBOL_GPL(seq_buf_path);
>  
>  /**
>   * seq_buf_to_user - copy the sequence buffer to user space
> diff --git a/lib/tests/seq_buf_kunit.c b/lib/tests/seq_buf_kunit.c
> index 046614100c77..5eaf024fa3a5 100644
> --- a/lib/tests/seq_buf_kunit.c
> +++ b/lib/tests/seq_buf_kunit.c
> @@ -7,7 +7,9 @@
>  
>  #include <kunit/test.h>
>  #include <linux/console.h>
> +#include <linux/fs.h>
>  #include <linux/seq_buf.h>
> +#include <linux/shmem_fs.h>
>  #include <linux/string.h>
>  
>  static void seq_buf_init_test(struct kunit *test)
> @@ -466,6 +468,47 @@ static void seq_buf_do_printk_test(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, seq_buf_printk_empty, 0);
>  }
>  
> +/* Long enough that it cannot fit in the room the test leaves for it. */
> +#define SEQ_BUF_TEST_PATH	"/seq_buf_kunit_path_name"
> +
> +static void seq_buf_path_overflow_test(struct kunit *test)
> +{
> +	DECLARE_SEQ_BUF(s, 32);
> +	const char *expected = "keep:xxxxxxxxxxxxxxxxxxxxxxx";
> +	struct file *file;
> +	size_t len;
> +	int i;
> +
> +	/*
> +	 * The tests run before anything writable is mounted, so take the file
> +	 * whose path gets printed from shmem, which needs no mount of its own.
> +	 */
> +	file = shmem_file_setup(SEQ_BUF_TEST_PATH, 0, EMPTY_VMA_FLAGS);
> +	if (IS_ERR(file))
> +		kunit_skip(test, "cannot create a file to print the path of");
> +
> +	/* Leave less room than the path needs, so d_path() cannot fit it. */
> +	seq_buf_puts(&s, "keep:");
> +	len = seq_buf_used(&s);
> +	for (i = len; i < 28; i++)
> +		seq_buf_putc(&s, 'x');
> +
> +	KUNIT_EXPECT_EQ(test, seq_buf_path(&s, &file->f_path, "\n"), -1);
> +	fput(file);
> +
> +	KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
> +
> +	/*
> +	 * d_path() keeps as much of the path as fits when it does not fit
> +	 * whole, and seq_buf_str() would hand out that fragment, as it ends
> +	 * the string at the last byte of an overflowed buffer.
> +	 */
> +	for (i = 28; i < 32; i++)
> +		KUNIT_EXPECT_EQ_MSG(test, s.buffer[i], '\0',
> +				    "byte %d past the data is not cleared", i);
> +	KUNIT_EXPECT_STREQ(test, seq_buf_str(&s), expected);
> +}
> +
>  static struct kunit_case seq_buf_test_cases[] = {
>  	KUNIT_CASE(seq_buf_init_test),
>  	KUNIT_CASE(seq_buf_declare_test),
> @@ -482,6 +525,7 @@ static struct kunit_case seq_buf_test_cases[] = {
>  	KUNIT_CASE(seq_buf_puts_partial_overflow_test),
>  	KUNIT_CASE(seq_buf_putmem_partial_overflow_test),
>  	KUNIT_CASE(seq_buf_putmem_hex_partial_overflow_test),
> +	KUNIT_CASE(seq_buf_path_overflow_test),
>  	KUNIT_CASE(seq_buf_do_printk_test),
>  	{}
>  };