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: 3549 bytes --]

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


Have a lovely night!
Alex

> 
> David
> 
> > +
> > +	seq_buf_init(&s, (char *)v->card, sizeof(v->card));
> > +	seq_buf_puts(&s, (const char *)tea->card);
> > +	seq_buf_puts(&s, " TEA5777");
> > +	seq_buf_terminate(&s);
> > +
> >  	strscpy(v->bus_info, tea->bus_info, sizeof(v->bus_info));
> >  	return 0;
> >  }
> > diff --git a/drivers/media/radio/tea575x.c b/drivers/media/radio/tea575x.c
> > index c37315226c42..e49228eb7c46 100644
> > --- a/drivers/media/radio/tea575x.c
> > +++ b/drivers/media/radio/tea575x.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/module.h>
> >  #include <linux/init.h>
> >  #include <linux/slab.h>
> > +#include <linux/seq_buf.h>
> >  #include <linux/sched.h>
> >  #include <asm/io.h>
> >  #include <media/v4l2-device.h>
> > @@ -221,10 +222,15 @@ static int vidioc_querycap(struct file *file, void  *priv,
> >  					struct v4l2_capability *v)
> >  {
> >  	struct snd_tea575x *tea = video_drvdata(file);
> > +	struct seq_buf s;
> >  
> >  	strscpy(v->driver, tea->v4l2_dev->name, sizeof(v->driver));
> > -	strscpy(v->card, tea->card, sizeof(v->card));
> > -	strlcat(v->card, tea->tea5759 ? " TEA5759" : " TEA5757", sizeof(v->card));
> > +
> > +	seq_buf_init(&s, (char *)v->card, sizeof(v->card));
> > +	seq_buf_puts(&s, (const char *)tea->card);
> > +	seq_buf_puts(&s, tea->tea5759 ? " TEA5759" : " TEA5757");
> > +	seq_buf_terminate(&s);
> > +
> >  	strscpy(v->bus_info, tea->bus_info, sizeof(v->bus_info));
> >  	return 0;
> >  }
> 
> 

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

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