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

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

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