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

[Kees Cook <kees@kernel.org>]

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

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

-- 
Kees Cook