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

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

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

Hi Kees,

> Date: 2026-10-22 11:06:32-0700
> From: 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;

Please think again about the possibility of using pointers instead of
lengths.  This is a source of off-by-one bugs that's only caused by
the choice of data, I believe.  If using pointers, truncation would be
stored as a NULL pointer, and off-by-ones would be less likely.

Consider this off-by-one to be just a sympthom of the underlying
problematic design.  Whenever a design forces you to write +1 or -1,
there are high chances that the design is sub-optimal regarding safety.

[...]
> > > +/**
> > > + * 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.)

Agree.


Have a lovely night!
Alex

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

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

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