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: 1762 bytes --]

Hi Steve,

> Date: 2026-10-29 10:18:38-0400
> From: Steven Rostedt <rostedt@goodmis.org>
>
[...]
> > > > > @@ -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.

Ugh!  This is another can of worms.  Do we need to accept users calling
_putmem() including a NUL terminator?  Perhaps we could ask users to not
include that NUL with _putmem() (and if they do so, then the buffer
might have two NULs, which isn't terrible).  Or perhaps _putmem() could
do .len-- if the last byte is a NUL.

Anyway, I'm disliking this API too much, and will probably work on my
own stpecpy()/seprintf() pointer-based pair of APIs to replace
strscpy(9)/snprintf(3), which are much simpler to use correctly, and
require much less churn than moving from strscpy(9)/snprintf(3) to
seq_buf.


Have a lovely night!
Alex

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

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