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

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

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

Hi Kees,

> Date: 2026-11-12 09:54:15-0800
> From: 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>
> > >
[...]
> > > >  	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.

I disagree that the whole C string API is confused.  In the future,
when array parameters become actual arrays (so you can use _Countof() on
them), you'll have better language support for that.

strlcat(3) indeed has some problems, and has to go, but I would like to
not put the whole C string API in that sack.

> 
> And I'd like _fewer_ APIs

That's a mistake, IMO.  The reason why programmers write bugs using the
string APIs is precisely because they try to use few APIs, and try to
use them even in places where a more appropriate API would be better.

Take for example strncpy(3).  It has been abused in so many ways, and we
all know the results of that.  Now look at what you've done in the
kernel: it's been replaced by around a dozen different APIs.  Quoting
the documentation:

	The replacements for strncpy() are:

	- strscpy() when the destination must be NUL-terminated.
	- strscpy_pad() when the destination must be NUL-terminated and
	  zero-padded (e.g., structs crossing privilege boundaries).
	- memtostr() for NUL-terminated destinations from non-NUL-terminated
	  fixed-width sources (with the `__nonstring` attribute on the source).
	- memtostr_pad() for the same, but with zero-padding.
	- strtomem() for non-NUL-terminated fixed-width destinations, with
	  the `__nonstring` attribute on the destination.
	- strtomem_pad() for non-NUL-terminated destinations that also need
	  zero-padding.
	- memcpy_and_pad() for bounded copies from potentially unterminated
	  sources where the destination size is a runtime value.

And that is the right thing to do.  In parallel, we've developed
fundamentally the same functions in shadow-utils.  I think that's where
research converges to: one API for each different use case.  That makes
the APIs more descriptive of the action they perform, which let's the
programmer better understand the code.  They're also way more robust, as
you can add many safety checks in the macros that implement them.

> (I realize that didn't happen at all for
> seq_buf, but that our current best direction).

I think the problem in seq_buf is not the amount of functions, but
rather the fact that it tries to support both strings and nonstrings at
the same time.  That's literally repeating the mistake of strncpy(3)
with this new type.  You may try to patch it with tape, but it's
fundamentally flawed, and it will eventually show up (we've already seen
the bugs in its implementation).

> And I especially don't
> want a "len" getting passed around.

I partially agree with this.  I don't like passing a length, because
lengths and sizes are too problematic.  However, I would like you to
trust me that pointers aren't that bad.  We've been using them in
shadow-utils extensively, and they're super-comfortable if the functions
are well designed.

> It needs bounds to be *in* the data
> type. And that means either true arrays,

Indeed, there's work towards that.  I'll show some preview of what may
come in future dialects of C.

Now, we can have a stpecpy() function:

	char *
	stpecpy(char dst[], const char end[0], const char *restrict src)
	{
		ssize_t  dlen;

		if (dst == NULL)
			return NULL;
		dlen = strscpy(dst, src, end - dst);
		if (dlen < 0)
			return NULL;

		return dst + dlen;
	}

In GCC, you can improve the prototype:

	char *
	stpecpy(char dst[], const char end[];
	    char dst[dst?end-dst:0], const char end[0], const char *restrict src)

With a patch I'm working on for GCC (it might land in GCC 17 if we're
lucky, but it's more likely to end up in GCC 18), we'll be able to use
_Countof() on array parameters, so we could use the two-arg version of
strscpy() for implementing this:

	char *
	stpecpy(char dst[], const char end[];
	    char dst[dst?end-dst:0], const char end[0], const char *restrict src)
	{
		ssize_t  dlen;

		if (dst == NULL)
			return NULL;
		dlen = strscpy(dst, src);
		if (dlen < 0)
			return NULL;

		return dst + dlen;
	}

And in a few years, we might end up having slices in C, which would
allow having the same functionality with just 2 arguments:

	char
	stpecpy(char dst[:], const char *restrict src)[:]
	{
		ssize_t  dlen;

		if (dst == NULL)
			return NULL;
		dlen = strscpy(dst, src);
		if (dlen == -1)
			return NULL;

		return dst + dlen;
	}

So, there's a bright future for traditional C string APIs.  And it's not
too far away.

> or internal bounds tracking
> (like seq_buf),

I believe that's going to bite eventually, and it will also be difficult
to return to the simplicity of the C string API once we have these few
features that make it safe.

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

No!  stpecpy() is really the same as strscpy(9).  It's *not* a cat
function.  See the implementation(s) above: there's no strlen(3) in it.

The closest relative to it is stpcpy(3) from POSIX, which is really just
strcpy(3), but returning a pointer that encodes the position.  That was
designed precisely to solve the performance issues of strcat(3).

> C strings were a mistake, but here we are.

Trust me: it's not.  The problems with C strings are, as you said above,
the limited support for array parameters in the language.  But it's not
too difficult to solve that problem.  And once that is solved, magically
the traditional string APIs become much better than the structure types,
by being at least as safe, but much simpler.

So, I'd like you to trust me to add stpecpy() --whatever we end up
calling it--, and perhaps the sprintf() variant of it, and see how it
really improves the code.  In the coming years, when we solve the issues
with array parameters in the language, they will become even better.

> 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


Have a lovely night!
Alex

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

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