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

[Steven Rostedt <rostedt@goodmis.org>]

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