seq_buf_str() silent truncation (data corruption)

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

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

Hi Steve,


I believe I've found a bug in seq_buf_str().

Consider the following code:

	char            buf[3];
	struct seq_buf  s;

	seq_buf_init(&s, buf, 3);
	seq_buf_putmem(&s, "foo", 3);  // This will return 0, I believe.

	// At this point, ->len==3 and ->size==3, I believe.

	seq_buf_str(&s);  // BUG

Let's see the implementation of seq_buf_str():

	$ git show v7.3:include/linux/seq_buf.h | grepc seq_buf_str
	(standard input):static inline const char *seq_buf_str(struct seq_buf *s)
	{
		if (WARN_ON(s->size == 0))
			return "";

		if (seq_buf_buffer_left(s))
			s->buffer[s->len] = 0;
		else
			s->buffer[s->size - 1] = 0;

		return s->buffer;
	}

The first check is fine (we don't trigger the WARN_ON()).
seq_buf_buffer_left(s) will return 0 (it returns 's->size - s->len').
This means we'll execute the second branch.  That branch will result in
overwriting the third byte with a '\0', without reporting the overflow.

I believe the following should fix the bug:

	@@ -102,10 +104,12 @@ static inline const char *seq_buf_str(struct seq_buf *s)
		if (WARN_ON(s->size == 0))
			return "";
	 
	-       if (seq_buf_buffer_left(s))
	+       if (seq_buf_buffer_left(s)) {
			s->buffer[s->len] = 0;
	-       else
	+       } else {
	+               seq_buf_set_overflow(s);
			s->buffer[s->size - 1] = 0;
	+       }
	 
		return s->buffer;
	 }

I have not yet researched enough to sign a patch, but please let me know
what you think.  I can prepare a formal patch next week.

NOT-assisted-by: LLM
Reported-by: Alejandro Colomar <alx@kernel.org>


Have a lovely night!
Alex

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

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