Re: [PATCH] media: radio: Replace strlcat() with seq_buf in TEA drivers

[David Laight <david.laight.linux@gmail.com>]

On Thu, 12 Nov 2026 09:54:15 -0800
Kees Cook <kees@kernel.org> wrote:

> On Sun, Nov 08, 2026 at 10:25:35PM +0100, Alejandro Colomar wrote:
> > Hi David,
> >   
> > > Date: 2026-11-08 21:02:32+0000
> > > From: David Laight <david.laight.linux@gmail.com>
> > >  
> > [...]  
> > > 
> > > seq_buf is just OTT.
> > > Using to replace a single strcat() is just silly.
> > > There is no point doing these patches until there is a sensible alternative.  
> > 
> > Agree.
> > 
> > [...]  
> > > >  	strscpy(v->driver, tea->v4l2_dev->name, sizeof(v->driver));
> > > > -	strscpy(v->card, tea->card, sizeof(v->card));
> > > > -	strlcat(v->card, " TEA5777", sizeof(v->card));  
> > > 
> > > What is wrong with (and almost OTT):
> > > #define TEA5777 " TEA5777"
> > > 	BUILD_BUG_ON(sizeof(TEA5777) <= sizeof(v->card))
> > > 
> > > 	len = strscpy(v->card, tea->card, sizeof(v->card) - sizeof(TEA5777) + 1);  
> > 
> > I can answer this one: bugs (particularly, off by one) are quite prone
> > in such code.
> > 
> > From the Authors of strlcat(3), you can look at OpenBSD's manual page:
> > 
> > 	Since it is known how many characters were copied the first
> > 	time, things can be sped up a bit by using a copy instead of an
> > 	append:
> > 
> > 		char *dir, *file, pname[PATH_MAX];
> > 		size_t n;
> > 
> > 		...
> > 
> > 		n = strlcpy(pname, dir, sizeof(pname));
> > 		if (n >= sizeof(pname))
> > 			goto toolong;
> > 		if (strlcpy(pname + n, file, sizeof(pname) - n) >= sizeof(pname) - n)
> > 			goto toolong;
> > 
> > 	However, one may question the validity of such optimizations, as
> > 	they defeat the whole purpose of strlcpy() and strlcat().  As a
> > 	matter of fact, the first version of this manual page got it
> > 	wrong.
> >   
> > > 	if (len < 0)
> > > 		goto fail;
> > > 	memcpy(v->card + len, TEA5777, sizeof(TEA5777);
> > > 
> > > What might be useful is a helper that would let you write:
> > > 	len = strscpy(v->card, tea->card);
> > > 	len = strscpy_at(v->card, len, " TEA5777" [, implied_length]);
> > > 	if (len < 0)
> > > 		goto fail;  
> > 
> > Indeed, as said in other threads, I have such an API:
> > 
> > 	p = v->card;
> > 	e = ARRAY_END(v->card);
> > 	p = stpecpy(p, e, tea->card);
> > 	p = stpecpy(p, e, " TEA5777");
> > 	if (p == NULL)
> > 		goto fail;

That one is playing with pointers.
They do make it harder to do any kind of compile-time (or compler helped)
sanity checks.

> > Linus didn't like my function name.  We could indeed call it strscpy_at().  
> 
> I really just don't want to have bare pointers or lengths being used for
> any of these manipulations. strlcat needs to go away because it's part
> of the whole C string API that is confused about array vs pointer and
> has no sense of handling bounds. Yes, FORTIFY does its best, but between
> lack of sane bounds and doing a strlen for every action, it has to go.

But both snprintf() and seq_buf are far more expensive than doing the
strlen().
There is the separate issue that strlcpy() manages to needlessly inline a
strnlen() in a lot of the call sites - instead of deferring it to the
helper function.

David

> 
> And I'd like _fewer_ APIs (I realize that didn't happen at all for
> seq_buf, but that our current best direction). And I especially don't
> want a "len" getting passed around. It needs bounds to be *in* the data
> type. And that means either true arrays, or internal bounds tracking
> (like seq_buf), and it shouldn't have "extra work" associated with it
> (e.g. stpecpy above still has to find the current end of the C string).
> C strings were a mistake, but here we are.
> 
> So, for "easy" constructions, sprintf-family, and for complex
> constructions, seq_buf. And probably the large bit of thought at each
> place where it is possible, is "how should truncation be communicated?"
> 
> -Kees
>