Re: [PATCH v5 06/12] seq_buf: Add seq_buf_strlen()

[Kees Cook <kees@kernel.org>]

On Fri, Oct 16, 2026 at 02:59:28PM -0400, Steven Rostedt wrote:
> On Mon,  5 Oct 2026 08:56:56 -0700
> Kees Cook <kees@kernel.org> wrote:
> 
> > +/*
> > + * NUL-terminate the buffer in @s: directly after the data when there is
> > + * room for it, otherwise in the last byte of the buffer. @s->size must not
> > + * be zero.
> > + *
> > + * Returns: the offset of the NUL.
> > + */
> > +static inline size_t __seq_buf_terminate(struct seq_buf *s)
> > +{
> > +	size_t end;
> > +
> > +	if (seq_buf_buffer_left(s))
> > +		end = s->len;
> > +	else
> > +		end = s->size - 1;
> > +
> > +	s->buffer[end] = 0;
> 
> Hmm, I just realized that this could inadvertently overflow the buffer. If
> len == size, it may not be overflowed. If using seq_buf_putmem() and
> filling the entire buffer, calling this would force an overflow as we just
> truncated the last byte.

Hm, yes. So the "else" branch needs to include:

	s->len = s->size + 1;

> 
> > +
> > +	return end;
> > +}
> > +
> >  /**
> >   * seq_buf_str - get NUL-terminated C string from seq_buf
> >   * @s: the seq_buf handle
> > @@ -98,7 +119,14 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
> >   *
> >   * Note, if this is called when the buffer has overflowed, then
> >   * the last byte of the buffer is zeroed, and the len will still
> > - * point passed it.
> > + * point passed it. The same happens when the buffer is exactly
> > + * full: the NUL takes the place of the last byte written, which is
> > + * lost, though seq_buf_used() still counts it.
> > + *
> > + * A zero-sized seq_buf has nowhere to put a NUL, so the empty string
> > + * is returned instead of writing to @s->buffer. Any other seq_buf
> > + * returns @s->buffer, even when it holds an empty string, so callers
> > + * always get their own buffer back.
> >   *
> >   * After this function is called, s->buffer is safe to use
> >   * in string operations.
> > @@ -107,17 +135,44 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
> >   */
> >  static inline const char *seq_buf_str(struct seq_buf *s)
> >  {
> > -	if (WARN_ON(s->size == 0))
> > +	if (s->size == 0)
> >  		return "";
> >  
> > -	if (seq_buf_buffer_left(s))
> > -		s->buffer[s->len] = 0;
> > -	else
> > -		s->buffer[s->size - 1] = 0;
> > +	__seq_buf_terminate(s);
> >  
> >  	return s->buffer;
> >  }
> >  
> > +/**
> > + * seq_buf_strlen - get the length of the NUL-terminated C string in seq_buf
> 
> Hmm,
> 
> I don't really like the way this modifies the string, as the request is
> just to return the string length.
> 
> Just like the above, this could cause an inadvertent overflow.
> 
> I could envision a use case where we want to use the entire buffer and do
> not expect it to be nul terminated. And this function could be used to get
> the length to print. That is, it may want to use the entire length.

Non-C String users would be using seq_buf_used(), not seq_buf_strlen().

> 
> Having "strlen()" modify the string just seems wrong to me.

No, this is absolutely required. It's nonsensical to work on a C String
without termination. (And it's the entire purpose of this series...)

> Is it really needed? If the user wants to make sure the string is
> terminated, it should use the next patch's "seq_buf_terminate()".

The ergonomics on this are very bad. I explicitly don't want to leave a
foot-gun around for this. :) (i.e. require all C String users also
remember to terminate it first; the whole point of the API is to have
the string always be safe.)

> And we need to update it so that if seq_buf_terminate() causes a
> truncation, it needs to set that it overflowed.

That I agree with. I will send a patch on top of the series since this
has been in -next for a while and the merge window is here now. :P

-- 
Kees Cook